shakacode / sass-resources-loader

SASS resources (e.g. variables, mixins etc.) loader for Webpack. Also works with less, post-css, etc.
MIT License
982 stars 66 forks source link

Add support for @use syntax #117

Closed Tomburgs closed 4 years ago

Tomburgs commented 4 years ago

Hey, ya'll! I added support for @use syntax in modules, it previously wouldn't have worked because it must be defined before any imports, mixins, functions, style rules and whatnot.

What this code does is it checks if @use syntax is used somewhere in the file, and if it is it will prepend the imports after it.


This change is Reviewable

justin808 commented 4 years ago

Hi @Tomburgs. Thank you for your PR.

Can you summarize how your PR differs from https://github.com/shakacode/sass-resources-loader/pull/111 ?

Are there ideas from #111 that should also be considered?

Also, can you expand the example to show that this new syntax works?

Thanks,

Justin

Tomburgs commented 4 years ago

Hey @justin808,

What this PR does is if the file it's going through contains @use statements, then it will move all of the resource code, which would typically be prepended, after the @use statements.

With the current implementation, any definitions inside of the resource file would not be usable from the imported file.

I would gladly take over PR #111, and combine it with my changes. It differs in a way that #111 resolves @use statements used inside of the resource file to use relative paths from the processed file.

justin808 commented 4 years ago

@Tomburgs

I would gladly take over PR #111, and combine it with my changes.

That would be great. If you can provide a sample and tests, much appreciated!

Tomburgs commented 4 years ago

Hi @justin808, I've updated this PR with changes from #111.

Some of the notable changes are:

I added tests for hoisting & import scenarios, and updated readme.

P.S @lucpotage perhaps you can have a look at this too, maybe I missed something.

lucpotage commented 4 years ago

Hi! Thank you for these great changes! However, I'm not sure I understand:

the original import syntax will always be preserved

The resources array does not indicate whether this module should opt for @use or @import. Does it mean it will force @use? If so, don't you think this is a breaking change and it is worth bumping to a major version (like 3.0.0)?

As for the hoistUseStatements, could you provide one example to better understand how it differs from the current behavior and why this option is required? πŸ™

Can't wait to using this new version.

Tomburgs commented 4 years ago

Hey @lucpotage, I am not sure if I understand what you mean by this

The resources array does not indicate whether this module should opt for @use or @import. Does it mean it will force @use?

The contents of each resource are prepended to each entry. If there's @import/@use used inside of any of the resource files, then its path is resolved to a relative path from the entry file. The regex used for this in #111 was this, which would mean that @import paths wouldn't be resolved:

/@use\s+(?:'([^']+)'|"([^"]+)"|([^\s;]+))/g

I updated it to this, so that this module would resolve both @import and @use:

/@(import|use)\s+(?:'([^']+)'|"([^"]+)"|([^\s;]+))/g

As for the hoistUseStatements option, it will simply move entry file's @use statements before the imported resources. This exists in cases where the imported resources are using @import / @mixin / @function and you wish to use @use from within the entry file. Without this option, an error would be thrown, since @use statements must come before any definitions other than @forward statements or variable definitions.

lucpotage commented 4 years ago

The contents of each resource are prepended to each entry.

I was not aware of this. I thought it prepended a line like @use 'resource-path' to each entry and not the content itself. πŸ˜… What you did makes totally sense especially since @import is still supported as of now.

As for the hoistUseStatements option, I got it, thanks. What about automatically hoisting use statements if any, since an error is thrown if we don't do it?

Tomburgs commented 4 years ago

As for the hoistUseStatements option, I got it, thanks. What about automatically hoisting use statements if any, since an error is thrown if we don't do it?

I chose to disable this option by default because if there's a case where a resource file has variables defined inside and the user wishes to pass them as with option, then the variable would be undefined. I believe this way it is more intuitive.

As for an error being thrown, it will only be thrown if there's a @import / @mixin / @function defined in the first level. A workaround for it could be to import files containing those using @use, but the hoisting approach might be more attractive to some.

lucpotage commented 4 years ago

If it's fine for you @justin808, it's your turn!

Tomburgs commented 4 years ago

@justin808, sorry for the late update, been stuck in vim.

I updated changelog, readme & provided some examples on options there.

justin808 commented 4 years ago

@Tomburgs Please:

  1. See my comments above
  2. Rebase on master
  3. Add a simple example of using @use

Alternatively, I could take over if you like. Please still answer my questions. I really appreciate your help thus far!

justin808 commented 4 years ago

@Tomburgs please let me know when you can do the updates. I'm very much looking forward to wrapping up this PR.