sass / dart-sass

The reference implementation of Sass, written in Dart.
https://sass-lang.com/dart-sass
MIT License
3.9k stars 352 forks source link

NodePackageImporter support for subpaths entry points without extensions #2183

Closed pascalduez closed 6 months ago

pascalduez commented 6 months ago

Greetings,

if we refer to the NodePackageImporter documentation at https://sass-lang.com/documentation/js-api/classes/nodepackageimporter, it should be possible to use subpaths entry points such as @use "pkg:uicomponents/colors"; with exports declared as:

{
  "exports": {
    ".": {
      "sass": "./src/scss/index.scss",
    },
    "./colors": {
      "sass": "./src/scss/_colors.scss",
    },
  }
}

I've tried every combination possible but couldn't make it work. I get the following error:

sass.Exception [Error]: Can't find stylesheet to import.                                                 

Here is a repro: https://github.com/pascalduez/sass-node-pkg-importer-repro

In fact, we can't find this precise case covered in the specs:
https://github.com/sass/sass-spec/blob/66778689d32c5559c399f910732e923f25574963/js-api-spec/node-package-importer.node.test.ts

Is it really supported?

Thanks!

jamesnw commented 6 months ago

Good catch, and thanks for the repro! It should be supported- in adding handling for the variants (./colors.scss, ./_colors.sass, etc.), I missed adding handling for the unaltered subpath.

That should be a straightforward fix. In the meantime, changing ./colors to ./colors.scss in your repro's package.json results in:

node_modules/@stuff/tokens/dist/colors.scss:1 Debug: colors file
nex3 commented 6 months ago

I know I already approved #3793, but thinking about it... is this behavior desirable? Or is this an issue with the documentation? Although Node.js does allow extensionless export specifiers for JS, they seem to encourage writing the extension. And because Sass will do extension resolution automatically on the export map, there's not necessarily a strong reason to allow both here.

jamesnw commented 6 months ago

I considered that, but I do think there's a case to be made for extensionless export specifiers. One of the motivations behind using conditional exports is to allow different types of exports for a path based on what is importing the path. Most of the time, that's going to be .js, but an export specifier may resolve different extensions- common within Node already would be .mjs, .cjs, and even .d.ts, and we're adding the potential for .css, .scss and .sass. The default export of "." is extensionless, as it needs to potentially resolve multiple extensions.

One concrete use case would be a component library that exposes a card component, and their exports would look something like-

{
  "exports": {
    "./card": {
      "sass": "./src/scss/_card.scss",
      "default": "./src/js/components/card.js",
    },
  }
}

We could potentially require a wildcard in the export key like ./card.*, with @use "pkg:foo/card.scss", but I think that would be unexpected to authors.

pkg:foo/card would no longer work, as it would have 3 matches (card.scss, card.sass, card.css). The current spec throws if multiple potential paths match to prevent ambiguity, and doesn't check matches for uniqueness. If we did decide to require a wildcard extension in these cases, we should probably update the spec there as well.

nex3 commented 6 months ago

But if using extensions is more idiomatic, wouldn't the better way to represent that be

{
  "exports": {
    "./card.scss": {
      "sass": "./src/scss/_card.scss",
    },
    "./card.js": {
      "default": "./src/js/components/card.js",
    },
  }
}

...since Node generally encourages users to write import "mypkg/card.js" instead of import "mypkg/card"?

I also think this may be easier to explain to users as "the exports field is a virtual filesystem that the importer looks at to load files" rather than "the exports field maps to the string in your URL, except that it also does a bunch of extra resolution like a filesystem, but unlike a filesystem it supports extensionless basenames".

ntkme commented 6 months ago

I think the exports can be considered to be very similar to symlinks. For a symlink, we would just load it at its location at face value, and don't really actually care about its realpath, that the symlink itself do need an extension, but realpath does not matter.

jamesnw commented 6 months ago

...since Node generally encourages users to write import "mypkg/card.js" instead of import "mypkg/card"?

I think the tension here is that this conflicts with Sass, which generally encourages @use 'mypkg/card';, and does a bunch of extra resolution for partials and extensions. So we need some mapping from authored URL to actual file path, and I think we're trying to figure out at what layer that conceptually happens.

I do agree that treating the exports field strictly as a virtual filesystem would be simpler, but we are already doing some extra resolution in the exports that differs from how Sass resolves on the filesystem with the default export. On a filesystem, the analogous concept would be an index file, but in exports, we resolve ".".

As far as Node recommendations vs practice, a few instances I found-

In my mind, it is surprising for "mypkg/card" not to resolve the specifier ./card, especially when Node would resolve a JS file for import "mypkg/card.

nex3 commented 6 months ago

You're probably right that this is easier to just allow than not since the parallel with JS is somewhat ambiguous. I do think we should update our documentation to exclusively show examples with explicit extensions, though.

nex3 commented 6 months ago

Fixed by https://github.com/sass/dart-sass/pull/2184.