dart-sass icon indicating copy to clipboard operation
dart-sass copied to clipboard

NodePackageImporter support for subpaths entry points without extensions

Open pascalduez opened this issue 1 year ago • 1 comments

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!

pascalduez avatar Feb 22 '24 09:02 pascalduez

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

jamesnw avatar Feb 22 '24 18:02 jamesnw

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.

nex3 avatar Feb 23 '24 00:02 nex3

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.

jamesnw avatar Feb 23 '24 01:02 jamesnw

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".

nex3 avatar Feb 23 '24 22:02 nex3

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.

ntkme avatar Feb 24 '24 00:02 ntkme

...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-

  • Vuetify exports both scss and css for the exensionless ./styles specifier
  • GovUK Frontend has .scss, .mjs and .js exports for the extensionless "." specifier

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.

jamesnw avatar Feb 26 '24 17:02 jamesnw

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 avatar Mar 05 '24 00:03 nex3

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

nex3 avatar Mar 05 '24 01:03 nex3