Skip to content

[Package Importer] Add types and specs#3728

Merged
nex3 merged 55 commits intosass:mainfrom
oddbird:feature.package-importer
Feb 6, 2024
Merged

[Package Importer] Add types and specs#3728
nex3 merged 55 commits intosass:mainfrom
oddbird:feature.package-importer

Conversation

@jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Oct 23, 2023

@jamesnw jamesnw requested a review from nex3 November 13, 2023 14:18
@jamesnw jamesnw requested a review from nex3 November 16, 2023 15:05
@jamesnw jamesnw requested a review from nex3 November 20, 2023 21:32
@jamesnw jamesnw marked this pull request as ready for review November 20, 2023 21:33
@jamesnw jamesnw requested a review from nex3 February 1, 2024 17:26
@nex3 nex3 mentioned this pull request Feb 1, 2024
6 tasks
Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

Since baseURL is now a directory rather than a file path, and the Node.js algorithms expect a file path, we should explicitly add a basename before passing it into those algorithms.

@jamesnw
Copy link
Contributor Author

jamesnw commented Feb 2, 2024

Since baseURL is now a directory rather than a file path, and the Node.js algorithms expect a file path, we should explicitly add a basename before passing it into those algorithms.

@nex3 Based on my reading of the algorithm, the parentURL
doesn't make any assertions on whether it is a file or directory. Passing a file in a directory is identical to passing in the directory, as parentURL is only used for relative resolution.

It's also unclear to me what we would use as a basename when the user only provides a directory.

@ntkme
Copy link
Contributor

ntkme commented Feb 2, 2024

I think what @nex3 means is that we can should pass /path/to/dir as /path/to/dir/.. Many times algorithm does not check if a path is a file or a directory, and this can potentially be dangerous. E.g. dirname('/path/to/dir') is different from dirname('/path/to/dir/.'), so that if an algorithm expect a file, it might be better to pass /path/to/dir/. to met the expectation.

@jgerigmeyer jgerigmeyer requested a review from nex3 February 6, 2024 17:13
@jamesnw
Copy link
Contributor Author

jamesnw commented Feb 6, 2024

I think what @nex3 means is that we can should pass /path/to/dir as /path/to/dir/.. Many times algorithm does not check if a path is a file or a directory, and this can potentially be dangerous. E.g. dirname('/path/to/dir') is different from dirname('/path/to/dir/.'), so that if an algorithm expect a file, it might be better to pass /path/to/dir/. to met the expectation.

@nex3 Does this match your expectations- adding . as a basename, and updating the spec to read-

  * Let `pkgImporter` be a [Node Package Importer] with an associated
    `entryPointURL` of the absolute file URL for `entryPointDirectory`
    appended with a [single-dot URL path segment](https://url.spec.whatwg.org/#single-dot-path-segment).

My reading of this is that this is only a change to the spec, and we don't need to update the implementation as well, correct?

@nex3
Copy link
Contributor

nex3 commented Feb 6, 2024

The parentURL parameter that's passed into PACKAGE_RESOLVE is ultimately used in two places:

  1. In PACKAGE_RESOLVE itself, node_modules/${packageSpecifier} is resolved relative to parentURL and then parentURL is set to dirname(parentURL).

  2. It's passed to LOOKUP_PACKAGE_SCOPE, which immediately sets it to its own directory name at the beginning of the loop.

None of these operations actually looks at the basename. Even for resolving the relative path, node_modules/foo relative to a/b/c is a/b/node_modules/foo—the fact that the basename was c doesn't matter. This means, as @ntkme points out, that you need to pass in something like a/b/c/. if you want to actually check within a/b/c. I don't know why the Node.js algorithm is written this way... it seems silly to me to pass in information that's just going to be thrown out, which is why I wanted to avoid doing the same thing in the Sass API.

All that said, I think the best place to compensate for a weird API like this is immediately before invoking it. Store the information "at rest" in a way that makes the most sense semantically—as the directory that we first check for node_modules—and only add /. immediately before invoking the Node.js algorithm.

My reading of this is that this is only a change to the spec, and we don't need to update the implementation as well, correct?

That's right. The implementation now avoids the issue entirely by always operating in terms of directories in the first place.

@jamesnw
Copy link
Contributor Author

jamesnw commented Feb 6, 2024

@nex3 Good point about appending /. immediately before invoking the algorithm. I added it in e2517ca.

@nex3 nex3 merged commit f6832f9 into sass:main Feb 6, 2024
@jgerigmeyer jgerigmeyer deleted the feature.package-importer branch February 7, 2024 15:46
nex3 pushed a commit that referenced this pull request Mar 27, 2024
Co-authored-by: Jonny Gerig Meyer <jonny@oddbird.net>
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

Successfully merging this pull request may close these issues.

4 participants