Skip to content
This repository has been archived by the owner on Apr 16, 2020. It is now read-only.

Disable default extensions only for relative resolution within package boundaries #4

Closed
wants to merge 3 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Aug 28, 2018

This is an alternative approach to #2 (and based off of that branch).

The principle here is that we don't want to lose the ability to support import 'pkg/x' from supporting default extension checks (eg the .mjs and .js fallbacks etc), and that we also don't want import '/path/to/pkg/x' to be different from the above - in that importing a package by a relative or absolute path should behave the same as importing a package through node_modules resolution.

This keeps these invariants by restricting the removal of automatic extensions only to the case of relative resolution within the same package, determined by the package.json package boundary.

For example, say we have packages at "/path/to/pkgA", "/path/to/pkgB", and that we have package.json files in each package as well as a package.json file in "/path/to/pkgA/subpkg".

Then the following apply:

  • import './x' in /path/tp/pkgA/index.mjs -> direct resolution only
  • import './subpkg' in /path/to/pkgA/index.mjs -> checks extensions and folder main
  • import './subpkg/x' in /path/to/pkgA/index.mjs -> checks extensions and folder main
  • import 'pkg' in /path/to/pkgA/index.mjs -> checks extensions and folder main
  • import '../pkgB' in /path/to/pkgA/index.mjs -> checks extensions and folder main
  • import '/path/to/pkgB' in /path/to/pkgA/index.mjs -> checks extensions and folder main
  • import '/path/to/pkgB' in /path/to/pkgB/index.mjs -> checks extensions and folder main (not a relative resolution)
  • import '../pkgB/x' in /path/to/pkgB/index.mjs -> direct resolution only

This, to me, retains all the valued use cases of default extensions, while creating a well-defined separation for internal imports.

MylesBorins and others added 3 commits August 28, 2018 19:01
this build on top of the limitations of the package name map proposal.
It removes file extensions and directory resolution in the resolver.

This is only currently limited for local development. When resolving
dependencies file extension and directory resolution will still work.

Refs: https://github.com/domenic/package-name-maps
guybedford

This comment was marked as off-topic.

ljharb

This comment was marked as off-topic.

@guybedford
Copy link
Contributor Author

guybedford commented Aug 28, 2018

@ljharb the approach here is exactly created to ensure no distinction between node_modules and your local project. Can you explain specifically which cases are the problem here?

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

@guybedford sorry, i read "This keeps these invariants by restricting the removal of automatic extensions only to the case of relative resolution within the same package, determined by the package.json package boundary." - maybe i misunderstood. I see "while creating a well-defined separation for internal imports." as well - i don't want any separation. "Internal" imports and "every other kind of import" should work identically, mostly following node's require algorithm.

@guybedford
Copy link
Contributor Author

@ljharb I see, so you don't want any differences at all, and for things to remain the same. I can understand that.

Basically, what this does, is that when doing import './x' or import '../x' from one module in a package to another module within the same package, automatic extension adding is disabled just for these cases.

What this buys us is compatibility with package maps for the ecosystem as a whole, since package maps cannot map relative extensions. This means that in 5 years, any npm package can be loaded in the browser with a suitable package map.

This PR avoids the inconsistencies of having packages behave differently whether they are in node_modules or not, while still providing the above benefit.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

That will be the case even with extension adding, because the package name map can add the extensions for you on relative paths too - it's already compatible with that.

@guybedford
Copy link
Contributor Author

That will be the case even with extension adding, because the package name map can add the extensions for you on relative paths too - it's already compatible with that.

@ljharb that is not the case. Package name maps cannot map relative extensions - they only apply to bare specifiers.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

@guybedford that seems like a feature it should support, tbh (all paths, not just bare ones) - one use case is premptively replacing all URLs in the graph to point from production to a CDN.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2018

Either way, I don't think node should be crippling part of the user experience to cater to an unfinished, unshipped web proposal that still has the possibility of adding features (like non-bare-import mappings)

devsnek

This comment was marked as off-topic.

@guybedford
Copy link
Contributor Author

This is no longer applicable.

@guybedford guybedford closed this Nov 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants