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

Node resolver: Refactor, add tests, and improve edge cases #159

Merged
merged 5 commits into from
Jul 15, 2021

Conversation

calebeby
Copy link
Member

@calebeby calebeby commented Jul 13, 2021

Closes #155

I went through https://nodejs.org/api/modules.html#modules_all_together, and tried to copy Node's commonJS resolution pretty closely.

Now the resolver is shared between the "extension resolver" and the "node modules resolver". Previously they did not share code, but since there is a lot of overlap, now they live in the same place and share code.

The "extension resolver" handles resolving ./asdf to ./asdf.ts, ./asdf/index.js, and reading ./asdf/package.json and resolving through there if it exists.

The "node modules resolver" handles resolving node_modules paths.

@@ -0,0 +1,118 @@
import type { Plugin, RollupCache } from 'rollup';
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing in here has changed, it was just moved out of npm-plugin

Comment on lines 64 to 67
./node_modules/foo/package.json {"main": "entry.js"}
./node_modules/foo/entry.js
`);
expect(await fs.resolve('foo')).toBe('./node_modules/foo/entry.js');
Copy link
Member Author

Choose a reason for hiding this comment

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

What I've done in the past for something like this is to make real files/folders for each test, but I like this better because it is easier to see the whole test in one place, and because changes are easier to make and will show up better in diffs, etc.

Comment on lines +221 to +222
test.todo('LOAD_PACKAGE_SELF');
test.todo('LOAD_PACKAGE_IMPORTS');
Copy link
Member Author

Choose a reason for hiding this comment

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

I left these tests out for now. Package imports are node's new way of creating "aliases" to files in your project, so you can do stuff like import "#utils instead of putting large relative paths. I didn't implement support for that yet. And package self refers to being able to import a package in itself (like how in this project we import "pleasantest" instead of import '../dist/...'). Support for that also isn't implemented yet.

Both are edge cases and I'll get to them eventually, or when someone asks for it

@gerardo-rodriguez
Copy link
Member

@calebeby I've started reviewing this, I'll finish reviewing it tomorrow. 🔍 👀

Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@calebeby calebeby merged commit a999340 into main Jul 15, 2021
@calebeby calebeby deleted the refactor-resolution branch July 15, 2021 18:57
@github-actions github-actions bot mentioned this pull request Jul 15, 2021
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.

Add unit tests for node resolver and fix edge case bugs
2 participants