Proposal: Package Importer and built-in Node implementation by jamesnw · Pull Request #3660 · sass/sass

jamesnw · 2023-08-18T15:38:54Z

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

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

ntkme · 2023-09-14T06:34:57Z

One thing that might be missing in the embedded protocol is the current working directory. I suppose we want to lookup node_modules starting from the directory containing the input file. However if the input is a string that does not have a URL, the lookup should start from the working directory. As for a persisted and reused compiler started previously, the host may have changed its directory and thus not having the same working directory. So I think we need to pass the working directory to compiler via embedded protocol as a context information for nodePkgImporter.

jamesnw · 2023-09-14T11:32:39Z

One thing that might be missing in the embedded protocol is the current working directory. I suppose we want to lookup node_modules starting from the directory containing the input file. However if the input is a string that does not have a URL, the lookup should start from the working directory. As for a persisted and reused compiler started previously, the host may have changed its directory and thus not having the same working directory. So I think we need to pass the working directory to compiler via embedded protocol as a context information for nodePkgImporter.

@ntkme Does the Containing Url proposal address this need? I think has been accepted but not implemented.

ntkme · 2023-09-14T15:20:38Z

The containing url proposal was to provide the containing url of a import to custom importer functions (from compiler to host). Here we're considering providing the working directory for input that does not have a url (from host to compiler).

E.g. In node REPL, you can run const sass = require('sass') in a working directory that has node_modules/sass, because node would begin the lookup from the current working directory. So in sass, if we're running a string compilation that does not have a URL (filename) like sass.compileString('@use "pkg:bootstrap";'), it should to be treated similarly I suppose.

nex3 · 2023-09-14T23:09:37Z

I agree with @ntkme, we need to have some way to pass in an entryPointURL to the package importer.

nex3 · 2023-09-14T23:10:54Z

proposal/package-importer.d.ts.md

+```proto
+message CompileRequest {
+  message Importer {
+    PackageImporter packageImporter = 14;


This should be a new member of the Importer.importer oneof, since it's a new type of importer. Also, it doesn't need index 14; the index just needs to be unique within the Importer record.

Ah, right, I forgot to update to mirror the move in the JS API from a flag to a Importer object.

I updated this in 8658bc3.

For the entryPointUrl, I added a NodePackageImporter message with a string entry_point_url. The other option I considered but rejected was flattening the object with a single key into-

oneof importer { string node_package_importer_entry_point_url = 4; }

The nested message style I went with would allow for easier future additions, but I'm fine with either option (or something else).

@ntkme Thanks for catching and explaining this- does this address the need?

ntkme · 2023-09-15T15:39:52Z

Do we want to support loading from NODE_PATH or GLOBAL_FOLDERS? For example, loading a package installed globally like npm install -g bootstrap.

In node NODE_PATH + GLOBAL_FOLDERS are searched after local node_modules. A few implementation details in node:

NODE_PATH + GLOBAL_FOLDERS can be retrieved from require("module").globalPaths.
Mutating NODE_PATH during node runtime does not affect the load path, however mutating require("module").globalPaths does affect the load path.
- This means if we want to be 100% compatible with node resolve we need to pass require("module").globalPaths in embedded protocol for each compilation to be accurate.

https://nodejs.org/api/modules.html#requireresolverequest-options
https://nodejs.org/api/modules.html#loading-from-the-global-folders
https://nodejs.org/api/modules.html#all-together
https://nodejs.org/api/esm.html#importmetaresolvespecifier

https://github.com/nodejs/node/blob/v20.0.0/lib/internal/modules/cjs/loader.js#L1397-L1425

@nex3 @jamesnw Any thoughts?

ntkme · 2023-09-15T16:23:16Z

proposal/package-importer.d.ts.md

+  * Let `pkgImporter` be a [Node Package Importer] with an associated
+    `entryPointURL` of `require.main.filename`.


When running node REPL require.main is null. Node has a hack internally to make the import work by adding the working directory as the local search path:

https://github.com/nodejs/node/blob/e7618fb5a5fc25d76b6474e2a6607f04fd6f10e0/lib/internal/modules/cjs/loader.js#L824-L832

I suppose we can set entryPointURL to file://${process.cwd()}/ (node the slash at the end is important), which will give us enough information on the compiler side.

@ntkme I updated this for the Embedded Protocol in 3e3c5f3. Thanks!

nex3 · 2023-09-16T00:14:28Z

My instinct is that trying to resolve from globally-installed packages will cause more headache than value for users. @alexander-akait what's the prior art here with sass-loader? Does it look for dependencies from NODE_PATH and GLOBAL_FOLDERS?

nex3 · 2023-09-18T20:54:46Z

proposal/package-importer.d.ts.md

+is instantiated with an associated `entry_point_url` set to the value of
+`file://${process.cwd()}/`.


This is confusing—if the entry_point_url is a field of the proto, isn't it up to the host to provide it rather than the spec?

Ah, yes. Does this need to be specified elsewhere for the host, or is that not part of the spec?

The JS host isn't part of the spec, except in that it has to follow the JS API spec.

Removed the host in d14aba8.

nex3 · 2023-09-20T23:29:24Z

Not hearing from @alexander-akait, I think the safest bet is to just not support global load paths for now. It'd be easier to add them later than it would be to remove them. @jamesnw can you add a design note documenting the rationale here?

jamesnw · 2023-09-21T01:14:40Z

Not hearing from @alexander-akait, I think the safest bet is to just not support global load paths for now. It'd be easier to add them later than it would be to remove them. @jamesnw can you add a design note documenting the rationale here?

I added this in e733b78.

ECMAScript modules (used with import) do not support NODE_PATHs, but it is still supported (but discouraged) in CommonJS modules (used with require()). So I documented the decision to choose ECMAScript resolution, and noted no NODE_PATH as a result of that.

I suspect that Webpack would use CommonJS resolution for sass-loader, so this may be a surprise for some. But because Node docs discourage using NODE_PATHS and there's a straightforward workaround with symlinks, I don't think we need to support it.

jamesnw · 2023-09-22T13:59:30Z

@nex3 I've made the changes we talked about yesterday, and this is ready for another review. Thanks!

jamesnw and others added 30 commits June 29, 2023 22:40

Add proposal for pkg: dependency imports

cae27d5

Remove broken link

a763eed

wrap

9e8ab52

fix link

27523a2

Package importer 2nd draft

14b5df6

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

b71dba5

review

2781c92

Clarify Vite comment

856e4c7

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

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

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

jamesnw added 2 commits September 11, 2023 11:17

Move to package importer object

6cdf76a

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

bb13a45

nex3 approved these changes Sep 13, 2023

View reviewed changes

Move NodePackageImporter option to importers_new_

827855a

jamesnw requested a review from nex3 September 14, 2023 01:37

Make nodePackageImporter a stubbed Importer

943b081

nex3 reviewed Sep 14, 2023

View reviewed changes

jamesnw added 2 commits September 15, 2023 09:32

Add entry point url to embedded protocol

8658bc3

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

971b7fc

ntkme reviewed Sep 15, 2023

View reviewed changes

Document Embedded Protocol

3e3c5f3

nex3 reviewed Sep 18, 2023

View reviewed changes

Remove embedded host details

d14aba8

Document ECMAScript resolution and NODE_PATHS decision

e733b78

jamesnw added 3 commits September 21, 2023 09:30

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

6812c8f

Ignore link failing in CI

fb90364

Revert to opaque object for nodePackageImporter

c8fcd99

nex3 merged commit d6c7d94 into sass:main Sep 22, 2023

jgerigmeyer deleted the package-loads branch September 25, 2023 17:59

jamesnw mentioned this pull request Oct 23, 2023

[Package Importer] Add types and specs #3728

Merged

nex3 mentioned this pull request Feb 1, 2024

Ship a built-in package importer #2739

Closed

6 tasks

		* Let `pkgImporter` be a [Node Package Importer] with an associated
		`entryPointURL` of `require.main.filename`.

		is instantiated with an associated `entry_point_url` set to the value of
		`file://${process.cwd()}/`.

Conversation

jamesnw commented Aug 18, 2023

Uh oh!

ntkme commented Sep 14, 2023

Uh oh!

jamesnw commented Sep 14, 2023

Uh oh!

ntkme commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nex3 commented Sep 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntkme commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nex3 commented Sep 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nex3 commented Sep 20, 2023

Uh oh!

jamesnw commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesnw commented Sep 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ntkme commented Sep 14, 2023 •

edited

Loading

ntkme commented Sep 15, 2023 •

edited

Loading

jamesnw commented Sep 21, 2023 •

edited

Loading