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

Fixes errors in the PnP spec #4724

Merged
merged 4 commits into from Aug 10, 2022
Merged

Fixes errors in the PnP spec #4724

merged 4 commits into from Aug 10, 2022

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Aug 9, 2022

What's the problem this PR addresses?

A couple of errors have been raised in evanw/esbuild#1985

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@arcanis arcanis marked this pull request as ready for review August 9, 2022 16:57
@merceyz
Copy link
Member

merceyz commented Aug 10, 2022

Doesn't seem like the spec takes into account paths on Windows, for example if

3. Otherwise, if `specifier` starts with "/", "./", or "../", then
is implemented as specified C:/foo will be treated as a bare identifier.

@merceyz
Copy link
Member

merceyz commented Aug 10, 2022

The spec doesn't mention the v2 implementation of "Virtual folders" that used $$virtual (changed in #2595), we should probably document it and why it was changed.

@arcanis arcanis merged commit 08ef87a into master Aug 10, 2022
@arcanis arcanis deleted the mael/pnp-spec-fixes branch August 10, 2022 12:38
merceyz pushed a commit that referenced this pull request Aug 10, 2022
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual

4. If `relativeUrl` matches `manifest.ignorePatternData`, then

1. Return **null**

5. For each `referenceMap` value in `manifest.packageRegistryData`
5. Let `relativeUrlWithDot` be `relativeUrl` prefixed with `./` or `../` as necessary
Copy link

Choose a reason for hiding this comment

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

FYI relativeUrlWithDot is never used. I'm assuming it's supposed to be used instead of relativeUrl below.

merceyz pushed a commit that referenced this pull request Sep 21, 2022
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual
merceyz pushed a commit that referenced this pull request Oct 5, 2022
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual
merceyz pushed a commit that referenced this pull request Nov 16, 2022
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual
merceyz pushed a commit that referenced this pull request Dec 20, 2022
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual
merceyz pushed a commit that referenced this pull request Feb 1, 2023
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual
merceyz pushed a commit that referenced this pull request Mar 16, 2023
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual
merceyz pushed a commit that referenced this pull request May 1, 2023
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual
merceyz pushed a commit that referenced this pull request Jun 1, 2023
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual
merceyz pushed a commit that referenced this pull request Nov 14, 2023
* Fixes errors in the PnP spec

* Explicitly suggests using `path.resolve` from the manifest location

* Adds a mention about path formats

* Adds a note about $$virtual
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.

None yet

3 participants