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.

Sign up for GitHub

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

Jump to bottom

fix(node-resolve): Respect if other plugins resolve the resolution to a different id #1181

Merged
merged 2 commits into from May 2, 2022
Merged

fix(node-resolve): Respect if other plugins resolve the resolution to a different id #1181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c88dc7c
fix(node-resolve): Respect if other plugins resolve the resolution to…
lukastaegert May 1, 2022
6a214fa
chore(node-resolve): Install pnpm 6 instead of 7
lukastaegert May 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/node-windows.yml
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ jobs:
node-version: ${{ matrix.node }}

- name: install pnpm
run: npm install pnpm -g
run: npm install pnpm@6 -g

- name: pnpm install
run: pnpm install --ignore-scripts
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
View file
Open in desktop
Expand Up @@ -43,7 +43,7 @@ jobs:

- name: Install pnpm
run: |
npm install pnpm -g;
npm install pnpm@6 -g;
echo node `pnpm -v`;

- name: Set Git Config
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/validate.yml
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ jobs:
run: git branch -f master origin/master

- name: Install pnpm
run: npm install pnpm -g
run: npm install pnpm@6 -g

- name: Sanity Check
run: |
Expand Down
5 changes: 5 additions & 0 deletions packages/node-resolve/src/index.js
View file
Open in desktop
Expand Up @@ -281,6 +281,11 @@ export function nodeResolve(opts = {}) {
if (resolvedResolved.external) {
return false;
}
// Allow other plugins to take over resolution. Rollup core will not
// change the id if it corresponds to an existing file
if (resolvedResolved.id !== resolved.id) {
return resolvedResolved;
Copy link
Member

@tjenkinson tjenkinson May 1, 2022 •
edited

Choose a reason for hiding this comment

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

Could we always just return this? Would it ever be different to the line below (when the id is the same)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it would. The most important difference is the moduleSideEffects flag that would be overridden. You will immediately see all tests with regard to sideEffects fail. While I also considered merging in the value of this flag, I think it makes more sense that way: If the id does not change, then we are still talking about the same file and since we cannot find out if a plugin or node-resolve did the resolving, we assume node-resolve is the source of truth (but still merge in meta because node-resolve does not set meta information).
If the id changes, then it is a different file and we are sure if was another plugin interfering. Therefore, we assume the other plugin is right and our side effect information may also be wrong as it referred to the original file.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. This sounds like something that might make sense in rollup core? So a plug-in could return a result and then have that resolved result run though all the other plugins, and do the necessary merging of the first plug-in result to a later plug-in result (I.e rules like if the first plug-in says there are side effects and a future one doesn’t set the side effects flag use the result from the first one)

Or if not in the core maybe a helper function that could wrap this hook? E.g something like withDelegationToOtherPlugins(resolveId)?

Copy link
Member Author

Choose a reason for hiding this comment

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

How would you decide what the correct logic is if you do not know what the plugins actually want to do? Using the id to decide whether to use the this.resolve result over the own result IS very specific to node-resolve IMO because for node-resolve, ids are tied to actual files on the disk via package.json. For other plugins, the decision about side effects may be tied to very different criteria like extension, filters, meta data, content inspection, who knows.
I do not feel good about moving it to core. And for a shared helper, we would first need at least one other plugin that does what node-resolve does. I think node-resolve is special as it is a plugin that tries to resolve nearly everything while most other plugins just resolve some special ids.

Copy link
Member

Choose a reason for hiding this comment

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

How would you decide what the correct logic is if you do not know what the plugins actually want to do?

I was wondering if there was a way it could be generic but maybe that doesn’t make sense

Was thinking if a later plug-in didn’t explicitly return a value for a property then that could signal to use the value from the initial one

I was wondering if the id check wouldn’t actually be needed then too

If a later plug-in did change the id but not provide side effect info then maybe using the side effect result from the first plug-in could not work well

Copy link
Member Author

Choose a reason for hiding this comment

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

If a later plug-in did change the id but not provide side effect info then maybe using the side effect result from the first plug-in could not work well

Exactly my thinking considering side effects in node-resolve are tied to files on the disk.

}
// Pass on meta information added by other plugins
return { ...resolved, meta: resolvedResolved.meta };
}
Expand Down
30 changes: 29 additions & 1 deletion packages/node-resolve/test/test.js
View file
Open in desktop
@@ -1,4 +1,4 @@
import { join, resolve } from 'path';
import { join, resolve, dirname } from 'path';

import test from 'ava';
import { rollup } from 'rollup';
Expand Down Expand Up @@ -581,3 +581,31 @@ test('passes on meta information from other plugins', async (t) => {
]
});
});

test('allow other plugins to take over resolution', async (t) => {
await rollup({
input: 'entry/main.js',
onwarn: failOnWarn(t),
plugins: [
nodeResolve(),
{
name: 'change-resolution',
resolveId(importee) {
if (importee.endsWith('main.js')) {
return {
id: join(dirname(importee), 'other.js'),
meta: { 'change-resolution': 'changed' }
};
}
return null;
},

load(id) {
const info = this.getModuleInfo(id);
t.is(info.id, join(__dirname, 'fixtures', 'entry', 'other.js'));
t.deepEqual(info.meta, { 'change-resolution': 'changed' });
}
}
]
});
});