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

@use & @include doesn't work by default with yarn 3 / berry #1765

Open
Lonli-Lokli opened this issue Jul 29, 2022 · 12 comments
Open

@use & @include doesn't work by default with yarn 3 / berry #1765

Lonli-Lokli opened this issue Jul 29, 2022 · 12 comments

Comments

@Lonli-Lokli
Copy link

Bug report

Actual Behavior

Build error with Yarn 3 & @use or @include usage

Expected Behavior

No build errors

How Do We Reproduce?

  1. Clone https://github.com/Lonli-Lokli/yarn_pnp_scss
  2. yarn install
  3. yarn build
✔ Browser application bundle generation complete.

./src/styles.scss - Error: Module build failed (from ./.yarn/__virtual__/sass-loader-virtual-01a772c5dc/0/cache/sass-loader-npm-12.6.0-19096ee50d-5d73a42858.zip/node_modules/sass-loader/dist/cjs.js):
SassError: Can't find stylesheet to import.
  ╷
5 │ @include meta.load-css('tippy.js/dist/tippy.css');
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  src\styles.scss 5:1  root stylesheet

Same code with @import works

// DO NOT WORK
@use 'sass:meta';

@include meta.load-css('tippy.js/dist/tippy.css');
@include meta.load-css('tippy.js/themes/light.css');
@include meta.load-css('tippy.js/animations/scale.css');

// WORKS
// @import 'tippy.js/dist/tippy.css';
// @import 'tippy.js/themes/light.css';
// @import 'tippy.js/animations/scale.css';
@blackPeppper
Copy link

plus is not reading my vars when i use meta.load

@Lonli-Lokli
Copy link
Author

More info from sass-loader webpack-contrib/sass-loader#1077
_meta.load-css doesn't support yarn PnP, sounds like this function dont's node.js logic resolution, that is why you have problems, but everything works fine with @import
we just register own importer (it supports aliases, PnP and more resolver options) and sass execute it on each @import/@use and etc, if something was not resolved it means sass doesn't run our importer (or has bugs)
_

@nex3
Copy link
Contributor

nex3 commented Aug 2, 2022

Can you provide a reproduction for this that just uses the Dart Sass API? Custom importers are expected to work the same with meta.load-css() as they do with @import or @use, but it's hard to tell what's going on when you have all these additional dependencies in play.

@Lonli-Lokli
Copy link
Author

I've tried to create a working npm & failing yarn repro, unfortunately I was not able to make npm work

https://github.com/Lonli-Lokli/dart-sass-yarn

It is failing with npm run build with error. Can you explain how this code should be modified to make npm work? @include works fine


ERROR in ./src/styles.scss (./node_modules/css-loader/dist/cjs.js!./node_modules/sass-loader/dist/cjs.js!./src/styles.scss)
Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: Can't find stylesheet to import.
  ╷
3 │ @include meta.load-css('~tippy.js/dist/tippy.css');
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  src\styles.scss 3:1  root stylesheet
SassError: SassError: Can't find stylesheet to import.
  ╷
3 │ @include meta.load-css('~tippy.js/dist/tippy.css');
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  src\styles.scss 3:1  root stylesheet
    at Object.loader (D:\samples\dart-sass-yarn\node_modules\sass-loader\dist\index.js:69:14)
 @ ./src/styles.scss 8:6-141 22:17-24 26:0-111 26:0-111 27:22-29 27:33-47 27:50-64
 @ ./src/index.js 1:0-23

@nex3
Copy link
Contributor

nex3 commented Aug 9, 2022

I'm asking for a reproduction that doesn't depend on Webpack. As far as I know, that build error is just coming from the Webpack importer itself.

@Lonli-Lokli
Copy link
Author

@nex3 Can you please explain how can I provide an example without webpack, with loading external files from node_modules?

@nex3
Copy link
Contributor

nex3 commented Aug 10, 2022

I want you to provide an example of an importer for which Dart Sass is clearly behaving incorrectly. Whether it loads a file from node_modules or not doesn't matter—Sass doesn't know anything about node_modules. The question is, is there a bug in the API Dart Sass exposes for plugins like webpack to provide custom import logic?

@alexeagle
Copy link

alexeagle commented Jul 27, 2023

The JS distribution of dart-sass on https://npmjs.com/sass uses require in Node.js to locate dependencies, but this depends on "hoisting" behavior where the package manager places those dependencies in the top-level node_modules folder. sass itself doesn't depend on those packages.

Yarn documents this: https://yarnpkg.com/advanced/rulebook/#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies.
The same problem exists in pnpm:
https://pnpm.io/blog/2020/10/17/node-modules-configuration-options-with-pnpm
To make the sass package work, you need the last "worst case" option in this blog post.

The latter is relevant to me because the JS Bazel rules use pnpm's layout, so they don't work with sass.

Here is a reproduction that depends only on the sass CLI itself, no third-party loader:

.npmrc:

hoist=false

main.scss:

@use '@angular/material' as mat;

package.json:

{
    "scripts": {
        "test": "sass --load-path=node_modules main.scss main.css"
    },
    "dependencies": {
        "@angular/animations": "^16.0.0 || ^17.0.0",
        "@angular/cdk": "16.1.6",
        "@angular/core": "16.1.7",
        "@angular/common": "^16.0.0 || ^17.0.0",
        "@angular/forms": "^16.0.0 || ^17.0.0",
        "@angular/platform-browser": "^16.0.0 || ^17.0.0",
        "rxjs": "^6.5.3 || ^7.4.0",
        "zone.js": "~0.13.0",
        "@angular/material": "16.1.6",
        "sass": "1.64.1"
    }
}

Now run npx pnpm i && npm test:

Error: Can't find stylesheet to import.
  ╷
1 │ @use '@angular/material' as mat;
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  main.scss 1:1  root stylesheet

alexeagle added a commit to aspect-build/bazel-examples that referenced this issue Jul 27, 2023
Works around sass/dart-sass#1765
so that the npmjs.com/sass package can run under hoist=false.
alexeagle added a commit to aspect-build/bazel-examples that referenced this issue Jul 27, 2023
Works around sass/dart-sass#1765
so that the npmjs.com/sass package can run under hoist=false.
alexeagle added a commit to aspect-build/bazel-examples that referenced this issue Jul 27, 2023
Works around sass/dart-sass#1765
so that the npmjs.com/sass package can run under hoist=false.
alexeagle added a commit to aspect-build/bazel-examples that referenced this issue Jul 27, 2023
Works around sass/dart-sass#1765
so that the npmjs.com/sass package can run under hoist=false.
@nex3
Copy link
Contributor

nex3 commented Jul 27, 2023

@alexeagle That looks like expected behavior to me. If you run the Sass CLI (or JS API) without any additional options, it will only support relative loads. It has no built-in support for Node.js-style node_modules resolution at all. (Note that sass/sass#2739 will change this eventually, but that proposal is still being drafted.)

Today, the only things that can connect Sass to node_modules directories are:

  1. Explicitly passing node_modules directories as load paths to the Sass compiler, either on the CLI or the JS API. In this case, if you want it to look through multiple node_modules directories, you have to pass each one explicitly.
  2. Using a third-party plugin (like sass-loader for WebPack) that implements a Sass importer which does follow the Node.js resolution algorithm. In this case, if resolution is behaving incorrectly, that's an issue with that plugin and not with Sass itself.

@alexeagle
Copy link

alexeagle commented Aug 30, 2023

Thanks for your detailed attention as always, @nex3

I believe there are actually two issues:

  1. Explicitly passing a node_modules directory to the Sass compiler is required, as you observe a better ergonomics for this is already WIP. The --load-path argument was missing from my repro above, I've corrected that.

Now you can use that repro to observe that with .npmrc containing

shamefully-hoist=true

that the repro command (npx pnpm i && npm test) executes successfully.

  1. With an empty .npmrc or with the hoist=false setting, it fails like so:
> test
> sass --load-path=node_modules main.scss main.css

Error: Can't find stylesheet to import.
  ╷
6 │ @use '@material/typography' as mdc-typography;
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  node_modules/@angular/material/core/typography/_typography.scss 6:1  @forward
  @angular/_index.scss 12:1                                            @use
  main.scss 1:1                                                        root stylesheet

The Node.js resolution algorithm is meant to begin the search in node_modules starting from the use-site of the require statement (called the parent in Node.js implementation). So, the @use statement in node_modules/@angular/material/core/typography/_typography.scss ought to resolve by walking up the tree and finding the node_modules/@angular/material/core/node_modules/ folder, where the dependencies of @angular/material are installed.

We can workaround this by adding another --load-path:

> sass --load-path=node_modules --load-path=node_modules/.pnpm/@angular+material@16.1.6_z2mdkzecpj6njk476kyznmcgpe/node_modules main.scss main.css

Error: Can't find stylesheet to import.
   ╷
33 │ @use '@material/focus-ring/focus-ring';
   │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

as you can see, this fixes our error but introduces the next one. A user is forced to exhaustively follow the third-party dependency structure to enumerate every possible node_modules subdirectory that may be referenced. Here's how it ends up looking to get a successful build with Bazel:
aspect-build/bazel-examples@1d77a3b#diff-1376ff1e5aa025dafc74ec5be35cbbd45fdd8670a46c62b798f02a640877bbe5

I think this issue #2 could be resolved with the existing --load-path flag, by adjusting how the resolutions are performed, but I can also understand why you'd want to keep the semantics dumb and wait for sass/sass#2739

@Lonli-Lokli
Copy link
Author

Just to note that there are other packages managers (yarn, pnpm) which has their own vision on node_modules resolve

@nex3
Copy link
Contributor

nex3 commented Sep 1, 2023

@alexeagle The right way to handle this for the time being (or any issue of the class "just passing a simple --load-path doesn't accurately capture my loading logic") is to implement a custom importer, or use one off-the-shelf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants