Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

External imports don't hot reload #3

Open
birkir opened this issue Jun 16, 2018 · 22 comments
Open

External imports don't hot reload #3

birkir opened this issue Jun 16, 2018 · 22 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@birkir
Copy link

birkir commented Jun 16, 2018

Consider the following example

Button.js

const styles = require('./Button.styl');
export default () => <View style={styles.button} />;

Button.styl

@import './src/styles/variables.styl';
.button {
  color: $my-color;
}

variables.styl

$my-color: red;

Changing $my-color in variables.styl won't hot reload the styles to Button until I add for example a newline into Button.styl and hit save.

@kristerkari kristerkari added the bug Something isn't working label Jun 16, 2018
@kristerkari
Copy link
Owner

Thanks @birkir!

The Sass version currently has the same limitation. React Native's packager does not know that the file that has changed is imported in another file, so it does not know how to reload it.

One possible fix would be to add some kind of logic that keeps track of which files are imported using @import and tell the transformer to reload the correct file.

@birkir
Copy link
Author

birkir commented Jun 16, 2018

Yes i thought of that, do you know if the transformer offers any method to rebuild other files in the same metro context.

As a first test I would like to attempt to rebuild all stylus files when one of them changes, if I can get that working I can try to make something that traverses the import tree to only update necessary files.

Other option I was thinking about, to just write some kind of external watcher script that would automatically fire an fsevent to mark other files as changed so the metro bundler would rebuild them

@kristerkari
Copy link
Owner

Thanks for helping to figure this out!

I haven't looked into Metro internals enough yet, so I don't know if it is possible to tell it to load multiple files on file change. What you should be at least able to do is to change the filename that gets loaded.

As a first test I would like to attempt to rebuild all stylus files when one of them changes, if I can get that working I can try to make something that traverses the import tree to only update necessary files.

I was thinking of just parsing the imports from the .styl file or then using an abstract syntax tree if Stylus has one available.

e.g. Button.styl has imports as ['./src/styles/variables.styl'], and when variables.styl changes, the packager should see that it is imported in another file and reload Button.styl and any other file that imports it.

I haven't tried that yet, so I'm not sure how well it would work.

Other option I was thinking about, to just write some kind of external watcher script that would automatically fire an fsevent to mark other files as changed so the metro bundler would rebuild them

That could work, but I would prefer not to spawn any other processes/scripts to do the live reloading, as it sounds like it would solve one problem but possible introduce other new problems.

@birkir
Copy link
Author

birkir commented Jun 16, 2018

Okay gotcha.

Do you know if any other preprocessor or postprocessor works with imported files? less, postcss etc.

I am implement theming in RN and require the "theme" externally is pretty mandatory for this to work.

@kristerkari
Copy link
Owner

Do you know if any other preprocessor or postprocessor works with imported files? less, postcss etc.

I don't think so, because this is a limitation that is coming from Metro bundler, not the post/preprocessor.

Not being able to hot load imported files is something that has been bothering me for quite long already, so I will try to spend some time today and tomorrow to figure out some kind of fix.

@birkir
Copy link
Author

birkir commented Jun 16, 2018

Yes the work you've done on css modules in react native is beyond spectacular - having HMR on imports is the final blocker for my implementation right now. This library will be a killer competition to styled components (and in my opinion superior).

Places where I've been looking:

I am not really sure if the invalidating of built modules is happening from metro-babel or metro-core.

@kristerkari
Copy link
Owner

Thank you for the kind words! It's always motivating to hear that people find something that I have done useful. 👍

Also thanks for the links, I need to really dig deep into Metro's source code, as the docs for Metro are missing most of the things.

@kristerkari
Copy link
Owner

kristerkari commented Jun 17, 2018

Alright, I spent a while looking at the problem.

The first issue that needs to be solved is that currently when you use @import Metro does not have any knowledge of the file that is imported.

So when you do a change in variables.styl, the bundler does not react to the change in any way as that file does not get imported in Javascript like Button.styl does.

I'm currently just trying to find out a way to tell Metro that there are additional files that it should watch. Maybe it's possible by changing some options from the transformer, or maybe I have to even go as far as adding require('./src/styles/variables.styl') to the CSS->JS transformation result that gets passed to React Native.

@kristerkari
Copy link
Owner

kristerkari commented Jun 17, 2018

The require('./src/styles/variables.styl') trick at least works. Metro will react to changes from variables.styl if I do that.

Now I need to create some kind of mapping that forces Button.styl to be re-rendered when I change variables.styl. The main problem with doing that is that I'm not able to have a shared global state between the files when they get loaded because Metro seems to load them in their own instances.

@birkir
Copy link
Author

birkir commented Jun 17, 2018

Allright, you seem to be on the right track! That's a clever trick, so in theory we could get a dependency tree of stylus imports and require them in the JS output.

Only thing i'm worried about is infinite loop because of recursive imports. So maybe just a limitation that we can document if we bring this idea to life.

Again, thanks for all your time!

@kristerkari
Copy link
Owner

kristerkari commented Jun 17, 2018

Yeah so I didn't have more time today to look at the fix after my last message, but I need to spend some more time to properly understand what Metro is doing under the hood.

I'm still looking for a better fix, but for now I'll first try to make it work using the require hack, and then look for a better solution.

Does Stylus support recursive imports? Can you give me an example of how that works?

@birkir
Copy link
Author

birkir commented Jun 18, 2018

No problem

Unrelated to the current issue I ended up with another solution
for the specific use-case I am having: theming

I used the already existing css3 variables syntax to reference variables

// Button.styl
.button {
  color: var(--primary-color);
}
// Themes.styl
.light {
  --primary-color: red;
}
.dark {
  --primary-color: blue;
}
// Button.js
import theme from './theme';
const styles = theme(require('Button.styl'));

export default () => (
  <View style={styles.button} />
);
// theme.js
// I know this is a mess, 

import kebabCase from 'lodash/kebabCase';
import mapKeys from 'lodash/mapKeys';
const themes = require('./themes.styl');

for (const name in themes) {
  // This is because stylus (or react-native-css-modules?)
  // changes css variables to camelCase: `-PrimaryColor`
  themes[name] = mapKeys(themes[name], (value, key) => `--${kebabCase(key)}`);
}

export default function theme(styles) {

  const cache = {};

  for (const key in styles) {
    cache[key] = {};
    for (const name in styles[key]) {
      const value = styles[key][name];
      cache[key][name] = value;
      if (typeof value === 'string') {
        const matchVar = value.match(/var\((.*?)\)/);
        if (matchVar) {
          const varName = matchVar[1].trim();
          for (const theme in themes) {
            if (themes[theme][varName]) {
              // Create new style with theme suffix
              const themeKey = `${key}__theme-${theme}`;
              const themeValue = themes[theme][varName];
              cache[themeKey] = cache[themeKey] || {};
              cache[themeKey][name] = themeValue;
            }
          }
          // Delete var()'s as they won't pass compilation in StyleSheet.create
          delete cache[key][name];
        }
      }
    }
  }

  // I like the statically allocated StyleSheet
  const sheet = StyleSheet.create(cache);
  const result = {};

  for (const key in sheet) {
    // Map all available styles (plus currently selected theme)
    // UI.theme is a mobx store in my instance that will update styles on change.
    result[key] = [sheet[key], sheet[`${key}__theme-${UI.theme}`]];
  }

  return res;
}

This way, HMR works like charm, styles are statically created with StyleSheet, and can be changed with

@kristerkari
Copy link
Owner

kristerkari commented Jun 18, 2018

That's a very nice workaround! That's even something that could be turned into a library.

// This is because stylus (or react-native-css-modules?)
// changes css variables to camelCase: -PrimaryColor

That happens because the CSS parser (https://github.com/styled-components/css-to-react-native) does not support custom prorperties yet. I can create a fix for the parser to avoid that issue.

EDIT:

The fix is really easy to implement:

screen shot 2018-06-18 at 11 37 47

@kristerkari
Copy link
Owner

Here is the PR to fix custom props support:
styled-components/css-to-react-native#85

@birkir
Copy link
Author

birkir commented Jun 18, 2018

This is great! thanks for the PR, I can turn this into a package and demonstrate how to use it with redux or Context.

@birkir
Copy link
Author

birkir commented Jun 19, 2018

Early version of the package and its use case:

https://github.com/birkir/react-native-css-modules-theme

@kristerkari
Copy link
Owner

@birkir That looks great! And I'm happy to see that you are using Typescript too.

In other news I got green light for my styled-component's CSS parser pull request, so I can release a new version of it later tonight.

One thing that came up is that the parser itself does not support using var(--my-prop) with CSS shorthand properties, but there is a workaround for that.

@kristerkari
Copy link
Owner

I'm looking again at fixing the hot reloading of @imports.

A really tricky thing is that Metro is running the files in different Node processes, which makes it really hard to share the knowledge of which file is imported by which file.

This is how it looks like when I log the filename and the pid:

transform[stdout]: filename: /Users/kristerkari/Git/CSSModulesExample/src/Buttons.scss
transform[stdout]: pid: 49926
transform[stdout]: filename: /Users/kristerkari/Git/CSSModulesExample/src/_ButtonColors.scss
transform[stdout]: pid: 49924

I would really like to avoid writing a file to disk to same that data, so I'm hoping that I can still find a way to do it in Metro bundler instead of creating hacks to work around the issue.

@birkir
Copy link
Author

birkir commented Jun 23, 2018

Yeah exactly. They must provide some kind of interface to do this, babel has to do the same thing in theory.

@birkir
Copy link
Author

birkir commented Aug 2, 2018

I would like to revisit this issue but I am not sure what we need to move forward. I can create a issue on the metro bundler where I ask for general approach how we could solve this, or, as you know way more about those things than me, have a more pinpointed issue about what we would need from metro.

I don't know if they have the API available, if it's super tied into babel or other blockers that could be in the way.

Thanks Krister

@kristerkari
Copy link
Owner

kristerkari commented Aug 2, 2018

I would like to revisit this issue but I am not sure what we need to move forward. I can create a issue on the metro bundler where I ask for general approach how we could solve this, or, as you know way more about those things than me, have a more pinpointed issue about what we would need from metro.

I guess you already know how it should work: we would need would be some way to tell Metro to reload another file when a file changes. So when variables.styl, then it should actually not only reload that file, but reload Button.styl too.

So we really need some way for Metro give better control of what file to load when a reload happens.

I tried to just swap the file contents and filename of variables.styl to Button.styl inside the transformer, but that does not seem to work.

I don't know if they have the API available, if it's super tied into babel or other blockers that could be in the way.

One of the problems I have had is that Metro's API has a lot of options, but does not have any API docs. I have tried to pass some options with rn-cli.config.js file in a React Native project through the bundler, but it seems that it ignores those options, even if some of them seem to be defined in React Native's source code. I have also tried to pass some options directly from the transformer file, but those seem to get ignored too.

I guess that the only way forward right now is to try to get some help from the people at Facebook who are working on React Native or Metro.

@kristerkari
Copy link
Owner

I had a look at the open issues on Metro repository. Very few of them have answers from the maintainers, but it seems that it's possible to get some help sometimes. Here's an example:
facebook/metro#176 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants