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

NodePackageImporter support for subpaths entry points without extensions #2183

Closed
pascalduez opened this issue Feb 22, 2024 · 8 comments
Closed
Assignees

Comments

@pascalduez
Copy link

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
Copy link
Contributor

jamesnw commented Feb 22, 2024

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 jamesnw changed the title NodePackageImporter support for subpaths entry points NodePackageImporter support for subpaths entry points without extensions Feb 22, 2024
@jamesnw jamesnw self-assigned this Feb 22, 2024
@nex3
Copy link
Contributor

nex3 commented Feb 23, 2024

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
Copy link
Contributor

jamesnw commented Feb 23, 2024

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
Copy link
Contributor

nex3 commented Feb 23, 2024

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
Copy link
Contributor

ntkme commented Feb 24, 2024

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
Copy link
Contributor

jamesnw commented Feb 26, 2024

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

@nex3
Copy link
Contributor

nex3 commented Mar 5, 2024

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
Copy link
Contributor

nex3 commented Mar 5, 2024

Fixed by #2184.

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

No branches or pull requests

4 participants