Add proposal for pkg: dependency imports #3

jamesnw · 2023-06-30T02:43:55Z

Description

Draft PR for a design for locating and loading Sass files from “pkg:” URLs with platform-specific semantics.

jgerigmeyer · 2023-06-30T15:29:10Z

I want to use pkg: URLs to load Sass dependencies on various platforms.

nex3 · 2023-06-30T22:59:00Z

proposal/package-importer.md

+the reader where to find the file.
+
+While the Dart Sass implementation allows for the use of the `package:` url
+scheme, a similar standard doesn't exist in Node. We could add `node_modules` to


The other major issue with node_modules is that a single file may have access multiple node_modules directories, and what's more different files may have access to different node_modules directories within the same compilation.

Addressed in 14b5df6

nex3 · 2023-06-30T23:01:37Z

proposal/package-importer.md

+the load path, but that again suffers from lack of clarity to the implementation
+and the reader.
+
+Some npm packages have adopted a convention of declaring their Sass entrypoints


There's a logical separation between design concerns for the platform-agnostic pkg: syntax and design concerns for the Node.js-specific package importer. Consider adding a heading here to visually separate out the latter.

nex3 · 2023-06-30T23:03:31Z

proposal/package-importer.md

+currently does not provide a native way to set a condition on a single
+invocation of `require.resolve`, meaning we would need to implement our own
+`package.json` parsing similar to [Rollup] or add a dependency on a community
+solution like [resolve.exports]. However, this proposal will not prevent future


I'm not intrinsically opposed to using something community-standard like resolve.exports, especially since it's so small.

nex3 · 2023-06-30T23:05:52Z

proposal/package-importer.md

+A potential source of confusion is in instances where a dependency defines a
+default or main export that is not in the root folder. In these cases, Sass will
+look for index and partial files in the same folder as the default or main
+export, and not in the package root.


Does this refer to a default or main JS export?

Can you document the reasoning behind this design decision as well?

This would be for any file defined as main or module, which for most packages would be a JS export. However, some packages do have other file extensions- see https://github.com/Decathlon/vitamin-web/blob/main/packages/sources/css/package.json#L21 for css. Bulma was the only instance of a sass main export I could find.

However, based on our conversation and discovery, I've reversed this and Sass will look relative to the package root instead of relative to the package main export.

proposal/package-importer.md

nex3 · 2023-06-30T23:27:05Z

proposal/package-importer.md

+New step for [Loading a Source File], after "if argument is a relative URL"
+step:
+
+- If `argument` is a valid URL with scheme `pkg`:
+  - Let `resolved` be the result of resolving a pkg url
+  - If `resolved` is not null, let `argument` equal `resolved`, and continue.
+    Otherwise, return null.


There are two ways we could go here for defining "package importers":

Reserve the URL scheme pkg: and dispatch it specially to a designated package importer, which is what this proposal does currently.

Allow any importer to resolve pkg: imports and just provide an official recommendation for the structure of pkg: URLs as well as a built-in importer for Node.js semantics.

The benefits of option 1 are that you can give package importers some assistance in resolving URLs (as you do below), and that you can enforce more stringent requirements on them (like only processing pkg: URLs) than you can on general importers.

On the other hand, option 2 gives authors a lot more flexibility. For example, you can imagine someone wanting to support pkg: URLs in the browser via import maps, or even just from a virtual filesystem in web IDE. And authors who do want to target the filesystem can mitigate the pain by writing FileImporters that handle the pkg: URL scheme.

So my recommendation is this: Don't define any new language semantics. Instead, define the expectations for an importer that handles pkg: URLs: that the first component will be the package name, that it will automatically try to find an entrypoint file, that nested files can be addressed using /, and so on.

proposal/package-importer.md

jamesnw · 2023-07-11T20:44:48Z

@nex3 I believe this second draft is ready for another review. I'm especially interested to hear if there are additional portions of the spec that I need to take into account and change. Thanks!

* main: [Calculation API] Fix some issues brought up during implementation (sass#3631) Bump tj-actions/changed-files from 37.0.5 to 37.1.0 (sass#3632) [Calc Functions] Match the spec's behavior better for percentages (sass#3630) Bump tj-actions/changed-files from 37.0.2 to 37.0.5 (sass#3627) Fix a reference to a dead link (sass#3625)

proposal/package-importer.md

js-api-doc/options.d.ts

nex3 · 2023-07-14T00:02:40Z

js-api-doc/options.d.ts

+   * Whether or not to enable the built-in package importer to resolve any url
+   * with the `pkg` scheme.


This should be clearer that "the built-in package importer" specifically follows the Node.js resolution logic. When the time comes to add this to the documentation, we should probably either write out or link to documentation of exactly how it locates Sass files (that is, the "exports" field and so on).

proposal/package-importer.md

proposal/package-importer.d.ts.md

jerivas · 2023-07-21T18:24:41Z

proposal/package-importer.d.ts.md

+
+The new `usePkgImporter` option will not be available in the [Legacy JS API].
+Third-party applications that don't support the modern API will be unable to use
+the built-in package importer. Some notable examples follow.


I had trouble getting into this last section without context that we're talking about Sass' legacy API. Hope this paragraph helps.

Co-authored-by: Ed Rivas <ed@jerivas.com>

jamesnw · 2023-07-24T16:20:05Z

@nex3 I think this is ready for another round of review. One thing to consider is when you think it is ready enough to close this PR, and open a new PR over on the sass/sass Github. Thanks!

nex3

It's fine to move this over to the public repo whenever you want. The official call for public comment happens between landing this PR and marking it as accepted anyway.

proposal/package-importer.d.ts.md

nex3 · 2023-07-28T00:31:55Z

proposal/package-importer.d.ts.md

+
+While the Dart Sass implementation allows for the use of the `package:` url
+scheme, a similar standard doesn't exist in Node. We chose the `pkg:` url scheme
+as it clearly communicates to both the user and compiler, and does not have


Clearly communicates what?

Replaced with "We chose the pkg: url scheme as it clearly communicates to both the user and compiler that the specified files are from a dependency. The pkg: url scheme also does not have known conflicts in the ecosystem."

nex3 · 2023-07-28T00:34:42Z

proposal/package-importer.d.ts.md

+[Parcel]:
+    https://github.com/parcel-bundler/parcel/blob/2d2400ded4615375ee6bd53ef77b4857ad1591dd/packages/transformers/sass/src/SassTransformer.js#L163


Style nit: We usually put even very long URL references on single lines.

nex3 · 2023-07-28T00:36:40Z

proposal/package-importer.d.ts.md

+    /**
+     * Whether or not to enable the built-in package importer to resolve any url
+     * with the `pkg` scheme. This importer follows Node.js resolution logic.
+     *
+     * @defaultValue `false`
+     * @category Input
+     */
+    usePkgImporter?: boolean;


I think we should be clear in both the option name and documentation that this is the built-in Node.js package importer. It's entirely possible we'll want to add other built-in package importers in the future.

Renamed to useNodePkgImporter

proposal/package-importer.d.ts.md

nex3 · 2023-07-28T01:44:11Z

proposal/package-importer.d.ts.md

+
+The new `usePkgImporter` option will not be available in the [Legacy JS API].
+Third-party applications that don't support the modern API will be unable to use
+the built-in package importer. Some notable examples follow.


Given that so many existing plugins still don't support the modern API (:skull:), I wonder if we should support the Node.js package importer for the legacy API. It might not be so hard: we could just say that if usePkgImporter is set, pkg: loads go through the pkg: importer and if that returns null we invoke the legacy importer logic.

jamesnw · 2023-08-18T15:40:40Z

Moved to sass#3660. I think all conversation from this PR is resolved.

jamesnw added 2 commits June 29, 2023 22:40

Add proposal for pkg: dependency imports

cae27d5

Remove broken link

a763eed

jgerigmeyer added 2 commits June 30, 2023 11:48

wrap

9e8ab52

fix link

27523a2

nex3 requested changes Jun 30, 2023

View reviewed changes

Package importer 2nd draft

14b5df6

jamesnw requested a review from nex3 July 11, 2023 20:43

jamesnw and others added 3 commits July 12, 2023 09:03

Merge branch 'main' of github.com:oddbird/sass into package-loads

b71dba5

review

2781c92

jgerigmeyer reviewed Jul 13, 2023

View reviewed changes

proposal/package-importer.md Outdated Show resolved Hide resolved

proposal/package-importer.md Outdated Show resolved Hide resolved

Clarify Vite comment

856e4c7

nex3 requested changes Jul 14, 2023

View reviewed changes

jamesnw and others added 14 commits July 14, 2023 12:25

review response

4bfa2fb

Move type into package-importer

7b30f49

Include md.d.ts files in toc scripts

358123a

Review response

3c3af5e

Remove Dart semantics

fd9fbd2

Add additional community examples

54e4039

Merge branch 'main' of github.com:oddbird/sass into package-loads

c565c93

Remove proposed option from options.d.ts

dadaa1d

Replace options with correct copy

9d549ee

Edits

7cc0f47

Add procedures section

82b3739

Break out pkg url resolution, and define Package importer results

a2a890d

Document exports resolution, make vs more consistent

2554f70

Add missing links

6e10182

jerivas reviewed Jul 21, 2023

View reviewed changes

jamesnw and others added 4 commits July 21, 2023 22:29

Add import to ts to satisfy ambient module error

f8b6f1c

Co-authored-by: Ed Rivas <ed@jerivas.com>

Standardize language

64a724e

Temporarily add debug info to link check

a4da3ac

Revert debugging in link check

031fec9

jgerigmeyer requested a review from nex3 July 26, 2023 18:54

nex3 requested changes Jul 28, 2023

View reviewed changes

jamesnw added 16 commits August 8, 2023 09:16

Merge branch 'main' of github.com:oddbird/sass into package-loads

d42ee46

Address review and style

daee9d9

Allow style at root to load sass and scss files. Fix JS option name.

5a026bb

Importer clarifications

949e1b3

Sort procedures by appearance

85ebde3

Updates to resolving package root values

5dbe6dc

Clean up root directory resolution

eff344d

Point Resolving package exports procedure to Node procedure

676c409

Point package root procedure to Node procedures

7422ab8

Give a name to input url

eb26657

Lint

2adad7c

Merge branch 'main' of github.com:oddbird/sass into package-loads

5ba76f7

Switch to pkgImporter: string option

be55d14

Add pkg importer support to legacy API

6c3025b

Add partial, extension and index matching on conditional export alias

b99d195

Fix typo

e926efa

jamesnw closed this Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add proposal for pkg: dependency imports #3

Add proposal for pkg: dependency imports #3

jamesnw commented Jun 30, 2023

jgerigmeyer commented Jun 30, 2023

nex3 Jun 30, 2023

jamesnw Jul 11, 2023

nex3 Jun 30, 2023

nex3 Jun 30, 2023

nex3 Jun 30, 2023

jamesnw Jul 11, 2023

nex3 Jun 30, 2023

jamesnw commented Jul 11, 2023

nex3 Jul 14, 2023

jerivas Jul 21, 2023

jamesnw commented Jul 24, 2023

nex3 left a comment

nex3 Jul 28, 2023

jamesnw Aug 8, 2023

nex3 Jul 28, 2023

nex3 Jul 28, 2023

jamesnw Aug 8, 2023

nex3 Jul 28, 2023

jamesnw commented Aug 18, 2023

		* Whether or not to enable the built-in package importer to resolve any url
		* with the `pkg` scheme.

		[Parcel]:
		https://github.com/parcel-bundler/parcel/blob/2d2400ded4615375ee6bd53ef77b4857ad1591dd/packages/transformers/sass/src/SassTransformer.js#L163

Add proposal for pkg: dependency imports #3

Add proposal for pkg: dependency imports #3

Conversation

jamesnw commented Jun 30, 2023

Description

jgerigmeyer commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesnw commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesnw commented Jul 24, 2023

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesnw commented Aug 18, 2023