kristerkari / react-native-sass-transformer

Use Sass to style your React Native apps.
MIT License
219 stars 19 forks source link

getSourceExts() breaks typical module resolution #5

Closed hypest closed 5 years ago

hypest commented 6 years ago

SCSS file gets loaded instead of the intended JS file.

Context

When exporting an getSourceExts() as per the README.md, the SCSS extensions take precedence before the normal js, json file extensions Metro looks up. See this relevant commit on the RN project.

When importing a local module without specifying an extension (perfectly valid use case as to allow for the platform specific file to get loaded) the bundler resolves the SCSS file first, the transformer produces JS code but the intended JS module is never imported. No error is raised until later, when some code tries to execute code from the "missed" module.

hypest commented 6 years ago

As a rather hacky workaround, one can define the getSourceExts() function as such:

module.exports = {
  getSourceExts() {
    return ["js", "json", "scss", "sass"];
  }
};

so the JS and JSON extensions still take precedence over the SCSS ones.

I wonder if there's a cleaner way to solve this issue. Ideas?

kristerkari commented 6 years ago

@hypest Thanks for this! I would really recommend to always specify the filetype when importing files.

I think that your workaround might be as good as it gets if you don't specify the file extension when importing.

I've been using the file extensions with imports in all my projects, and with the help of babel-plugin-react-native-platform-specific-extensions I have been also using platform specific extensions.

EDIT: Oh I see what you mean now. It used to work in React Native and has now changed and with that it broke the imports.

kristerkari commented 6 years ago

btw. I had a look at the gutenberg-mobile project, and it has quite a big transformer file inside the project.

Please let me know about any issues you have using the default react-native-sass-transformer. Let's fix the issues here, so that people don't have to create their own workarounds 👍

kristerkari commented 6 years ago

I guess that for now I'll just update the docs with your workaround, and let's see if it's possible to do a better fix for the issue.

It sucks that React Native gives such limited options for specifying options for Metro.

hypest commented 6 years ago

Oh, thanks for checking out gutenberg-mobile @kristerkari!

It's 100% my plan to contribute that transformer back here! The main added feature implemented there is the ability to resolve SCSS @import statements into platform specific imports. Think: importing 'variables.native.scss' if present.

The other feature added was to auto-imports some SCSS files, simulating a similar feature sass-loader offers in webpack.

Currently, the transformer needs extracting a few configuration options to make it independent of the gutenberg-mobile project, at which point it will be ready to be contributed upstream.

Hope that makes sense and open to suggestions on how to bring those features back upstream if you think it's useful and appropriate. ❤

kristerkari commented 6 years ago

I'm definitely open for any new features that can be added to the sass transformer. 👍

So far what has been holding me back has been lack of time and the fact that I haven't found a good way to pass options to node-sass. The good news is that node-sass is open for adding a project config file (https://github.com/sass/node-sass/issues/1144#issuecomment-372022394), which would allow the options to be defined outside of the transformer.

Just ping me if you want to discuss any specific features or if you have something ready that you want to contribute.

hypest commented 6 years ago

I guess that for now I'll just update the docs with your workaround, and let's see if it's possible to do a better fix for the issue.

Oh, thanks! Btw, let me know if there's a cleaner way to rewrite this issue's description as it apparently wasn't super clear.

It sucks that React Native gives such limited options for specifying options for Metro.

True. We find ourselves resorting to Babel plugins to customize behavior which might be more suitable for Metro, the bundler.

kristerkari commented 5 years ago

This is now properly solved in React Native 0.57+ where you have to add your extensions to the existing ones:

sourceExts: [...sourceExts, "scss", "sass"]