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

Add proposal for pkg: dependency imports #3

Closed
wants to merge 43 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
cae27d5
Add proposal for pkg: dependency imports
jamesnw Jun 30, 2023
a763eed
Remove broken link
jamesnw Jun 30, 2023
9e8ab52
wrap
jgerigmeyer Jun 30, 2023
27523a2
fix link
jgerigmeyer Jun 30, 2023
14b5df6
Package importer 2nd draft
jamesnw Jul 11, 2023
b71dba5
Merge branch 'main' of github.com:oddbird/sass into package-loads
jamesnw Jul 12, 2023
38abde0
Merge branch 'main' into package-loads
jgerigmeyer Jul 13, 2023
2781c92
review
jgerigmeyer Jul 13, 2023
856e4c7
Clarify Vite comment
jamesnw Jul 13, 2023
4bfa2fb
review response
jamesnw Jul 14, 2023
7b30f49
Move type into package-importer
jamesnw Jul 17, 2023
358123a
Include md.d.ts files in toc scripts
jamesnw Jul 17, 2023
3c3af5e
Review response
jamesnw Jul 21, 2023
fd9fbd2
Remove Dart semantics
jamesnw Jul 21, 2023
54e4039
Add additional community examples
jamesnw Jul 21, 2023
c565c93
Merge branch 'main' of github.com:oddbird/sass into package-loads
jamesnw Jul 21, 2023
dadaa1d
Remove proposed option from options.d.ts
jamesnw Jul 21, 2023
9d549ee
Replace options with correct copy
jamesnw Jul 21, 2023
7cc0f47
Edits
jamesnw Jul 21, 2023
82b3739
Add procedures section
jamesnw Jul 21, 2023
a2a890d
Break out pkg url resolution, and define Package importer results
jamesnw Jul 21, 2023
2554f70
Document exports resolution, make vs more consistent
jamesnw Jul 21, 2023
6e10182
Add missing links
jerivas Jul 21, 2023
f8b6f1c
Add import to ts to satisfy ambient module error
jamesnw Jul 22, 2023
64a724e
Standardize language
jamesnw Jul 24, 2023
a4da3ac
Temporarily add debug info to link check
jamesnw Jul 24, 2023
031fec9
Revert debugging in link check
jamesnw Jul 24, 2023
d42ee46
Merge branch 'main' of github.com:oddbird/sass into package-loads
jamesnw Aug 8, 2023
daee9d9
Address review and style
jamesnw Aug 8, 2023
5a026bb
Allow style at root to load sass and scss files. Fix JS option name.
jamesnw Aug 8, 2023
949e1b3
Importer clarifications
jamesnw Aug 8, 2023
85ebde3
Sort procedures by appearance
jamesnw Aug 8, 2023
5dbe6dc
Updates to resolving package root values
jamesnw Aug 8, 2023
eff344d
Clean up root directory resolution
jamesnw Aug 8, 2023
676c409
Point Resolving package exports procedure to Node procedure
jamesnw Aug 9, 2023
7422ab8
Point package root procedure to Node procedures
jamesnw Aug 9, 2023
eb26657
Give a name to input url
jamesnw Aug 11, 2023
2adad7c
Lint
jamesnw Aug 15, 2023
5ba76f7
Merge branch 'main' of github.com:oddbird/sass into package-loads
jamesnw Aug 18, 2023
be55d14
Switch to pkgImporter: string option
jamesnw Aug 18, 2023
6c3025b
Add pkg importer support to legacy API
jamesnw Aug 18, 2023
b99d195
Add partial, extension and index matching on conditional export alias
jamesnw Aug 18, 2023
e926efa
Fix typo
jamesnw Aug 18, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions js-api-doc/options.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,15 @@ export interface Options<sync extends 'sync' | 'async'> {
*/
style?: OutputStyle;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @defaultValue `false`
* @category Input
*/
usePkgImporter?: boolean;
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

/**
* By default, Dart Sass will print only five instances of the same
* deprecation warning per compilation to avoid deluging users in console
Expand Down
260 changes: 260 additions & 0 deletions proposal/package-importer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,260 @@
# Package Importer

*([Issue](https://github.com/sass/sass/issues/2739))*

This proposal adds Sass support for a standard package importer, introducing a
`pkg` URL scheme to indicate Sass package imports in an implementation-agnostic
format.

## Table of contents

* [Background](#background)
* [Summary](#summary)
* [Design Decisions](#design-decisions)
* [Node Resolution Decisions](#node-resolution-decisions)
* [Semantics](#semantics)
* [Resolving a `pkg:` URL](#resolving-a-pkg-url)
* [Platform-Specific Semantics](#platform-specific-semantics)
* [Dart](#dart)
* [Node](#node)
* [Resolving `pkg` Root](#resolving-pkg-root)
* [Resolving `pkg` Subpath](#resolving-pkg-subpath)
* [Resolution Order](#resolution-order)
* [Deprecation Process](#deprecation-process)
* [Examples](#examples)
* [Node](#node-1)
* [Dart](#dart-1)
* [Ecosystem Notes](#ecosystem-notes)

## Background

> This section is non-normative.

Historically, Sass has not specified a standard method for using packages from
dependencies. A number of domain-specific solutions exist using custom importers
or by specifying a load path. This can lead to Sass code being written in a way
that is tied to a specific domain and make it difficult to rely on dependencies.

## Summary

> This section is non-normative.

Sass users often need to use styles from a dependency to customize an existing
theme or access styling utilities.

This proposal defines a `pkg` URL scheme for usage with `@use` that directs an
implementation to resolve a URL within a dependency. The implementation will
resolve the dependency URL using the standard resolution for that environment.
Once resolved, this URL will be loaded in the same way as any other `file:` URL.

For example, `@use "pkg:bootstrap";` would resolve to the path of a
library-defined export within the `bootstrap` dependency. In Node, that would be
resolved within `node_modules`, using the [Node resolution algorithm]. In Dart,
that would be resolved within `pub-cache`, using [package-config].

[node resolution algorithm]: https://nodejs.org/api/packages.html
[package-config]: https://pub.dev/packages/package_config

To better understand and allow for testing against the recommended algorithm, a
[Sass pkg: test] repository has been made with a rudimentary implementation of
the algorithm.

[Sass pkg: test]: https://github.com/oddbird/sass-pkg-test

### Design Decisions

We could use the `~` popularized by Webpack's `load-sass` format, but this has
been deprecated since 2021. In addition, since this creates a URL that is
syntactically a relative URL, it does not make it clear to the implementation or
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 chose the `pkg:` url scheme
as it clearly communicates to both the user and compiler, and does not have
known conflicts in the ecosystem.
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

The `pkg` scheme will not be supported in the browser version of Dart Sass. To
jamesnw marked this conversation as resolved.
Show resolved Hide resolved
support a similar functionality, a user would need to ensure that files are
served, and the loader would need to fetch the URL. In order to follow the same
algorithm for [resolving a file: URL], we would need to make many fetches. If we
instead require the browser version to have a fully resolved URL, we negate many
of this spec's benefits. Users may write their own custom importers to fit their
needs.

[resolving a file: URL]: ../spec/modules.md#resolving-a-file-url

The `pkg` import loader will be exposed through an opt-in option as it adds the
potential for file system interaction to `compileString` and
`compileStringAsync`.
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

#### Node Resolution Decisions

The current recommendation for resolving packages in Node is to add
`node_modules` to the load paths. We could add `node_modules` to the load paths
by default, but that lacks clarity to the implementation and the reader. In
addition, a file may have access to multiple `node_modules` directories, and
different files may have access to different `node_modules` directories in the
same compilation.

There are a variety of methods currently in use for specifying a location of the
default Sass export for npm packages. For the most part, packages contain both
JavaScript and styles, and use the `main` or `module` root keys to define the
JavaScript entry point. Some packages use the `"sass"` key at the root of their
`package.json`. Others have adopted [conditional exports], which is supported by
Vite.
jgerigmeyer marked this conversation as resolved.
Show resolved Hide resolved

[conditional exports]: https://nodejs.org/api/packages.html#conditional-exports

Because use of conditional exports is flexible and recommended for modern
packages, this will be the primary method used within Node. We will support both
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"within Node" -> "for the Node package resolver". We should be clear about the distinction between "resolving pkg: URLs within Node" and "resolving pkg: URLs using the Node-style algorithm", because we'll support both using other pkg: resolvers in Node (by allowing users to author them) and using the Node algorithm outside of Node (when compiling from the Dart CLI).

the `"sass"` and the `"style"` conditions, as Sass can also use the CSS exports
exposed through `"style"`.

## Semantics

This proposal defines a new importer that implementations should make available.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear what it means to "define a new importer" that's disabled by default when it doesn't have intrinsic semantics separate from the platform. I suggest separating this out into two distinct concepts:

  1. Define requirements for all package importers. They should all handle non-canonical URLs with scheme pkg, they should all treat the first path segment as a package name, they should all treat following path segments as paths within the package. Make it clear that these are expectations for both user-authored package importers and package importers provided by implementations.

  2. Define the behavior of one specific package importer (specifically, the one that uses the Node resolution algorithm), similar to how we define the behavior of a filesystem importer. We could even localize this definition to the spec for usePkgImporter.

It will be disabled by default. The loader will handle URLs:

- with the scheme `pkg`
- followed by `:`
jamesnw marked this conversation as resolved.
Show resolved Hide resolved
- followed by a package name
- optionally followed by a path, with path segments separated with a forward
slash.

### Resolving a `pkg:` URL

The `pkg importer` will provide a method to canonicalize a `pkg:` URL, and
extend the implementation's existing File Importer.

### Platform-Specific Semantics

#### Dart
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

Given `url` with the format `pkg:.*`:

- Let `path` be `url` without the `pkg` scheme
- Use [package-config] to resolve `path`

[package-config]: https://pub.dev/packages/package_config

#### Node
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

Given `url` with the format `pkg:.*`:
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

- Let `fullPath` be `url` without the `pkg` scheme
jamesnw marked this conversation as resolved.
Show resolved Hide resolved
- Let `resolved` be the result of using [resolve.exports] to resolve `fullPath`
with the `sass` condition set
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can just say "run this code" as part of a specification, especially since this will be implemented in places that won't necessarily have access to the library itself (like the Dart CLI). Does Node have some documentation of its resolution algorithm we can link to here?

(It's also worth noting that resolve.exports wouldn't even be sufficient—it allows us to go from a package.json file to a specific path within that package, but it doesn't tell us how to find the correct package.json file in the first place.)

- If `resolved` has the scheme `file:` and an extension of `sass`, `scss` or
`css`, return it.
jamesnw marked this conversation as resolved.
Show resolved Hide resolved
- Let `resolved` be the result of using [resolve.exports] to resolve `fullPath`
with the `style` condition set.
- If `resolved` has the scheme `file:` and an extension of `css`, return it.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to allow .sass and .scss files here? It's not obvious to me, so either way we should probably document this as a design decision.

Copy link
Author

@jamesnw jamesnw Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and forth on that, as I was only seeing css for style in existing packages, but it's probably more likely to be a surprise if it's inconsistent, so I reversed course. I added this to the Design Decisions-

While in practice, "style" tends to be used solely for css files, we will support scss, sass and css files for either "sass" or "style".

- Let `packageName` be the package identifier, and `subPath` be the path without
the package identifier.
jamesnw marked this conversation as resolved.
Show resolved Hide resolved
- If `subPath` is empty, return result of [resolving `pkg` root].
- Otherwise, return result of [resolving `pkg` subpath].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth adding a non-normative note (represented by a blockquote) that this importer doesn't resolve underscores, extensions, or index files—which is to say, authors need to explicitly list the full resolved path of the file they're loading.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 I added a note on this. One tricky piece is that in some cases we are using the Resolving a file URL so it will resolve underscores and extensions. That allows pkg:foo to resolve to /node_modules/foo/_index.scss, or pkg:foo/bar to resolve to /node_modules/foo/bar/index.sass without exports being set. It also allows pkg:foo with the sass tag at the package.json root of ./src/scss to resolve to /node_modules/foo/scss/index.sass. Is that too confusing? Should we require full resolved paths even when we could support resolution?

jamesnw marked this conversation as resolved.
Show resolved Hide resolved

[resolve.exports]: https://github.com/lukeed/resolve.exports
jgerigmeyer marked this conversation as resolved.
Show resolved Hide resolved
[resolving pkg root]: #resolving-pkg-root
[resolving pkg subpath]: #resolving-pkg-subpath

#### Resolving `pkg` Root
jamesnw marked this conversation as resolved.
Show resolved Hide resolved
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

- Let `packagePath` be the file path to the package root.
- Let `sassValue` be the value of `sass` in the `package.json` at the pkg root.
- If `sassValue` is a relative path with an extension of `sass`, `scss` or
`css`, return the `packagePath` appended with `sassValue`.
- Let `styleValue` be the value of `style` in the `package.json` at the pkg
root.
- If `styleValue` is a relative path with an extension of `css`, return the
`packagePath` appended with `styleValue`.
- Otherwise return the result of [resolving a file url] with `packagePath`.

[resolving a file url]: ../spec/modules.md#resolving-a-file-url

#### Resolving `pkg` Subpath

- Let `packagePath` be the file path to the package root.
- Let `fullPath` be `subpath` resolved relative to `packagePath`.
- Return the result of [resolving a file url] with `fullPath`.

#### Resolution Order
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

This algorithm resolves in the following order:

1. `sass` condition in package.json `exports`
1. `style` condition in package.json `exports`
1. If no subpath, then find root export:
1. `sass` key at package.json root
1. `style` key at package.json root
1. `index` file at package root, resolved for file extensions and partials
1. If there is a subpath, resolve that path relative to the package root, and
resolve for file extensions and partials
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

## Deprecation Process

The `package` url scheme supported in Dart is not part of the Sass spec, but is
supported by the `dart-sass` implementation when running in Dart. This should be
deprecated in favor of moving authors to the new `pkg` url scheme. Usage of the
`package` syntax will result in a deprecation message, and a future major
version of Sass should remove support.
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

## Examples
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

### Node
jamesnw marked this conversation as resolved.
Show resolved Hide resolved

For library creators, the recommended method is to add a `sass` conditional
export key as the first key in `package.json`.

```json
{
"exports": {
".": {
"sass": "./dist/scss/index.scss",
"import": "./dist/js/index.mjs",
"require": "./dist/js/index.js"
}
}
}
```

Then, library consumers can use the pkg syntax to get the default export.

```scss
@use "pkg:library";
```

### Dart

For Dart libraries to take advantage of this, they can place a file at
`lib/_index.scss`. Other files within the `lib` folder would also be available
for library consumers. For example, if a library `libraryName` contains
`lib/themes/dark/_index.scss`, a consumer could write `@use
"pkg:libraryName/themes/dark";`.

More examples can be found in the [Sass pkg: test] example repo.

## Ecosystem Notes

Vite is currently using the Legacy JS API, and has an [open issue] to update to
the modern API. They also do not expose Sass options to the user, so would need
to enable the `usePkgImporter` on their behalf or expose some configuration.

[open issue]: https://github.com/vitejs/vite/issues/7116

Webpack's [sass-loader] allows users to opt in to the modern API and exposes
Sass options to users.

For Rollup, [rollup-plugin-sass] uses the Legacy JS API. They do expose Sass
options to the user.

[rollup-plugin-sass]: https://github.com/elycruz/rollup-plugin-sass

It may be worth adding a [Community Conditions Definition] to the Node
Documentation. [WinterCG] has a [Runtime Keys proposal specification] underway
in standardizing the usage of custom conditions for runtimes, but Sass doesn't
cleanly fit into that specification.

[Community Conditions Definition]: https://nodejs.org/docs/latest-v20.x/api/packages.html#community-conditions-definitions
[WinterCG]: https://wintercg.org/
[Runtime Keys proposal specification]: https://runtime-keys.proposal.wintercg.org/#adding-a-key
16 changes: 15 additions & 1 deletion spec/js-api/options.d.ts.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {PromiseOr} from './util/promise_or';
* [`sourceMap`](#sourcemap)
* [`sourceMapIncludeSources`](#sourcemapincludesources)
* [`style`](#style)
* [`usePkgImporter`](#usepkgimporter)
* [`verbose`](#verbose)
* [`StringOptionsWithoutImporter`](#stringoptionswithoutimporter)
* [`syntax`](#syntax)
Expand Down Expand Up @@ -144,7 +145,7 @@ throwing an error. The following values are considered invalid:

* A `SassFunction` whose `signature` field isn't a valid Sass function signature
that could appear after the `@function` directive in a Sass stylesheet.

```ts
functions?: Record<string, CustomFunction<sync>>;
```
Expand Down Expand Up @@ -239,6 +240,19 @@ Implementations may support any subset of `OutputStyle`s, provided that:
style?: OutputStyle;
```

#### `usePkgImporter`

If true, the compiler will use the built-in [package importer] to resolve any
url with the `pkg` scheme.

[package importer]: ../../proposal/package-importer.md

Defaults to false.

```ts
usePkgImporter?: boolean;
```

#### `verbose`

If true, the compiler must print every single deprecation warning it encounters
Expand Down