Add support for @use syntax

[Tomburgs]

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 

[justin808]

Hi @Tomburgs. Thank you for your PR.

Can you summarize how your PR differs from #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]

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]

@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]

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

Some of the notable changes are:

• Instead of adding a rule option, the original import syntax will always be preserved, so that it's possible to use both @use & @import simultaneously in resource files.
• I added an option hoistUseStatements which if true will move entry files' @use statements before the resource block (false by default).

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]

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]

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]

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]

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]

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

[justin808]

I did a quick review. Apologies in advance if I missed some things in the discussions that applied to the code.

That being said, I think we can take some of the discussion above and improve the README.

BTW, thank you so much @lucpotage and @Tomburgs for this work!!!!

Reviewed 6 of 18 files at r3.
Reviewable status: 6 of 18 files reviewed, 8 unresolved discussions (waiting on @Tomburgs)

---

CHANGELOG.md, line 5 at r3 (raw file):

## [2.1.0] - 2020-07-22
* Supports the new Sass module system released with Dart Sass 1.23.0. The use of @use is recommended instead of @import.

Let's mention the added option of hoist.

We need to link to your PR and the related issue.

---

README.md, line 48 at r3 (raw file):

|----------------------|----------------------|-------------|----------------------------------------------------|
| `resources`          | `{String\|String[]}` | `undefined` | Resources to include in files                      |
| `hoistUseStatements` | `{Boolean}`          | `false`     | If true, entry file `@use` imports will be hoisted |

I don't know what hoisted is. I googled for it and I didn't find much.

Can we provide some reasons why and why not use this?

---

README.md, line 52 at r3 (raw file):

## Tips
* Do not include anything that will be actually rendered in CSS, because it will be added to every imported Sass file.
* Avoid using Sass `@use` rules inside resources files as it slows down incremental builds. Add imported files directly in `sassResources` array in webpack config instead. If you concerned about location of your resources index, you might want to check out the solution outlined in [this comment](https://github.com/shakacode/sass-resources-loader/issues/46#issuecomment-335211284).

should we mention "Sass @use or @import"?

---

README.md, line 53 at r3 (raw file):

* Do not include anything that will be actually rendered in CSS, because it will be added to every imported Sass file.
* Avoid using Sass `@use` rules inside resources files as it slows down incremental builds. Add imported files directly in `sassResources` array in webpack config instead. If you concerned about location of your resources index, you might want to check out the solution outlined in [this comment](https://github.com/shakacode/sass-resources-loader/issues/46#issuecomment-335211284).
* If you still want to use Sass `@use` rules make sure your paths are relative to the file they defined in (basically, your file with resources), except the ones started with `~` (`~` is resolved to `node_modules` folder).

same comment above

---

src/utils/processResources.js, line 5 at r3 (raw file):

const useRegex = '^@use .*\n?$';
const useRegexTest = new RegExp(useRegex, 'm');
const useRegexReplace = new RegExp(`${useRegex}(?![sS]*${useRegex})`, 'gm');

what is the [sS] part of the regex?

can you provide some descript here on the regexps?

---

src/utils/processResources.js, line 15 at r3 (raw file):

  }

  return `${resources}\n${source}`;

can I see a before and after?

As in source would be some small example files.
resources would be...

the result would be...

---

src/utils/processResources.js, line 25 at r3 (raw file):

  const stringifiedResources = Array.isArray(resources) ? resources.join('\n') : resources;
  const output = getOutput(source, stringifiedResources, options);

The old code had a check for an array or no array. Is that present in the new code?

---

test/scss/hoist2.scss, line 9 at r3 (raw file):

        color: secret.always-blue();
    }
}

how is this file different than hoist.scss? and hoist3.scss?

[justin808]

@lucpotage @Tomburgs Should we update: https://github.com/shakacode/sass-resources-loader/blob/master/example/app/assets/styles/mixins.scss

and the example to maybe use hoist?

and update the dependencies?

have we verified that there is no migration needed for this update?

Reviewed 12 of 18 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Tomburgs)

[Tomburgs]

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

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

[justin808]

Awesome @Tomburgs. I left a few comments. Very close!

Also, do I need to run this personally before making the release? In other words, is this well tested by you and @lucpotage?

@lucpotage any comments?

Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Tomburgs)

---

README.md, line 48 at r4 (raw file):

If true, entry file `@use` imports will be hoisted

If true, entry file `@use` imports will be hoisted. This means the @use statements will go above the inclusion of resources.

@Tomburgs per the docs, isn't this required? and if so, why is it an option?

---

README.md, line 75 at r4 (raw file):

#### `hoistUseStatements`
Tells the compiler if an existing `@use` statement is found in entry file, it should be hoisted to the top.

The reason is that `@use` must go before style rules, per the [docs](https://sass-lang.com/documentation/at-rules/use)

---

README.md, line 94 at r4 (raw file):

Then the output, with hoistUseStatements set to true would be the following

Then the output, with hoistUseStatements set to true would be the following. Note that the `@use` statements are above the inclusion of resources.

---

src/utils/processResources.js, line 8 at r4 (raw file):

const useRegexTest = new RegExp(useRegex, 'm');
// Makes sure that only the last instance of `useRegex` variable is found
const useRegexReplace = new RegExp(`${useRegex}(?![sS]*${useRegex})`, 'gm');

I am not following why you might an optional number of s or S before the ${useRegex}?

---

src/utils/processResources.js, line 14 at r4 (raw file):

    return source.replace(
      useRegexReplace,
      imports => `${imports}\n${resources}`,

imports is confusing?

Would this var name be better as useStatements?

    return source.replace(
      useRegexReplace,
      useStatements => `${useStatements}\n${resources}`,

[justin808]

@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!

[Tomburgs]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @justin808, @Tomburgs, and @use)

---

README.md, line 48 at r4 (raw file):

@Tomburgs per the docs, isn't this required? and if so, why is it an option?

It's not required, there can be variable declarations and other @use imports before the entry file's @use imports.
i.e if the resources only have variable declarations and @use imports, it would be fine if this is false.

---

src/utils/processResources.js, line 8 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I am not following why you might an optional number of s or S before the ${useRegex}?

Good catch, this was a typo, they were supposed to be operators \s and \S.

---

src/utils/processResources.js, line 14 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

imports is confusing?

Would this var name be better as useStatements?

    return source.replace(
      useRegexReplace,
      useStatements => `${useStatements}\n${resources}`,

You're right, will change it.

[justin808]

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

[justin808]

Thanks @Tomburgs

Reviewed 6 of 7 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Tomburgs)