Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Fix path fragment inputs #229

Merged
merged 1 commit into from Jun 25, 2019

Conversation

bterlson
Copy link
Contributor

This PR addresses #228 by prefixing path fragments with ./ before passing to node resolve. This makes some sense as node will not resolve modules by file path unless they start with /, ./, or ../. With this change, the new test passes.

Unfortunately, this PR regresses the test generates manual chunks which now throws Could not resolve entry module (simple). I'm not sure what the cause of this is, but since I may not be back for a while, putting this PR up in case it helps anyone else. Feel free to close if it's not helpful.

@bterlson
Copy link
Contributor Author

The issue with this PR is that resolveId isn't used exclusively for resolving module imports found in code. It's also used to resolve any modules specified in manualChunks. So the check for importer === undefined is incorrect. I'd hoped to use it as a sort of "are we importing a root of a module graph" check.

@lukastaegert
Copy link
Member

I a way, manual chunks start their own module graphs where the actual manual chunk that is created is the intersection between the graph created by the manual chunk entries and the graph created by the "normal" entry points. Somewhat confusing but allows for putting all modules in a package into the same manual chunk even when only deep imports are used.

@lukastaegert
Copy link
Member

lukastaegert commented Jun 23, 2019

Maybe in the situation where the importer is undefined we could do a two-pass check: If prefixing the module with ./ finds a module, use that one, otherwise assume it is an absolute path and resolve again.

@bterlson
Copy link
Contributor Author

@lukastaegert That's a good idea, I'll take a stab at implementing it. I'm curious: do you call a path like "foo/bar" an absolute path? I've been calling it a path fragment or an implicit relative path, but I'm no path wizard.

@lukastaegert
Copy link
Member

I think there are places in the code where such a path is referred to as "absolute". I know that is definitely not the correct name!

When resolving an import name that doesn't look like a relative or
absolute path, first attempt to resolve the corresponding relative path
before resolving the original import name.
@bterlson bterlson marked this pull request as ready for review June 24, 2019 15:53
@bterlson
Copy link
Contributor Author

bterlson commented Jun 24, 2019

I've implemented the two-pass lookup suggested by @lukastaegert above. I unified this somewhat with my previous PR #223 which was also implementing a two-pass lookup, so the logic is more clear now. I think this is ready for review!

@bterlson bterlson changed the title WIP fix path fragment inputs Fix path fragment inputs Jun 24, 2019
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

This is really nice, thanks so much ❤️

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.

None yet

2 participants