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

Fix bug when tracing/uninstalling the base package. #206

Merged
merged 7 commits into from
Feb 10, 2023

Conversation

Bubblyworld
Copy link
Contributor

Fixes #204. The bug is basically that when linking the base package itself,
the only thing the tracemap does is record the base package as a pin. When
you then try and load the resulting input map, the base package specifier is
treated as a custom user override, since it maps to a filepath and the
extraction algorithm doesn't know how to handle this.

Verified

This commit was signed with the committer’s verified signature.
pentschev Peter Andreas Entschev

Verified

This commit was signed with the committer’s verified signature.
pentschev Peter Andreas Entschev

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…map extraction.
Comment on lines 261 to 264
const exportSubpath = parsedTarget && await resolver.getExportResolution(pkgUrl, subpath, key);
if (exportSubpath) {
const exportSubpath = await resolver.getExportResolution(pkgUrl, subpath, key);

// If the plain specifier resolves to a package on some provider's CDN,
// and we can find a suitable export for the module, then we can lock
// the package down to an exact version:
if (parsedTarget && exportSubpath) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to keep the existing logic unless there is a good reason to change it?

// Otherwise if the plain specifier is a reference to the base package
// containing the import map, we can safely ignore it, as the tracemap
// will always resolve such imports correctly at the top-level:
const pcfg = await resolver.getPackageConfig(pkgUrl);
Copy link
Member

Choose a reason for hiding this comment

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

Rather than pkgUrl we should try operate on a version of getPackageBase scoped to the first package.json boundary only to match Node.js resolution semantics.

// containing the import map, we can safely ignore it, as the tracemap
// will always resolve such imports correctly at the top-level:
const pcfg = await resolver.getPackageConfig(pkgUrl);
if (pcfg && pcfg.name === parsedKey.pkgName) {
Copy link
Member

Choose a reason for hiding this comment

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

We also need to handle subpath and environment deconstruction here like all other resolutions.

@Bubblyworld
Copy link
Contributor Author

Tests seem rather flaky - can't reproduce the failures locally on either v16 or v14 🤔

@guybedford guybedford merged commit 4e9dbcf into jspm:main Feb 10, 2023
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.

Linking and uninstalling self fails to remove top-level import.
2 participants