fix (dev-server-rollup): support child plugins in resolution algorithm #2050
Conversation
|
|
Thank you so much for working on this @43081j ! On to the third bug you mentioned that is part of the commonjs module itself, filed here. That bug is only relevant for plugins that have code like this in them, and I am still unsure as to why this only fails in wds, while rollup seems to bundle this code just fine. Further investigation is required and should also be tackled in a separate PR, if it turns out to be an issue on the wds side. |
|
yep we can't quite do what was in your patch, though. it causes a test to fail (correctly). thats what the remaining infinite loop is: node-resolve in some cases calls commonjs and vice versa, indefinitely. we can't really store a trace of plugins on our side because the plugins have nothing in common (they don't share a context or anything for an individual resolution). i'm not sure yet how to solve that... basically the one piece of missing code is the skipSelf logic, this isn't an alternative, it just isn't implemented yet (inf loop still exists). |
|
to expand on that: by overriding rollup's resolution, we have to handle however, it isn't as simple as storing some set of plugins we've already visited.
somewhere, we do need to track the plugins we've already visited and skip them in that case. im just unsure where and what to tie it to (its a cache really, we need to bind that to something unique to each initial resolve call). i've tried so many combinations of caches, keying strategies, etc and they always fix the inf loop but cause the tests to fail. this is because they all over-cache, as far as i can see (for the reasons above). somehow we need to create a cache on the first |
|
@daKmoR FYI this seems to reproduce the problem: it('can transform modules which resolve commonjs dependencies', async () => {
const rootDir = path.resolve(__dirname, '..', 'fixtures', 'basic');
const { server, host } = await createTestServer({
plugins: [
{
name: 'test',
serve(context) {
if (context.path === '/foo.js') {
return 'import {expect} from "chai"; export {expect};';
}
},
},
commonjs(),
nodeResolvePlugin(rootDir, false, {}),
],
});
try {
const text = await fetchText(`${host}/foo.js`);
console.log(text);
} finally {
server.stop();
}
});in the it causes an inf loop, probably in master too |
|
ok i pushed a change now which tries to respect the the reason it can't live inside instead, i've cached in this seems to work though im not entirely sure if keying by don't think anyone exists to review this, maybe @lucaelin if you remember the stuff you investigated you might follow it. or someone can maybe just try it out? i've tried it with chai which was a prime example of the inf loop problem, and it seems to work. |
|
Correctly passing Lets look at it in individual steps:
I think a better flow would be this:
The changes required would be:
The issue I have with that though, is that passing What do you think? I might be completely wrong about this :D |
|
im not entirely sure. every plugin i've seen within rollup expects to pass the options all the way through, so i expected the same here. e.g. here the commonjs plugin is telling the rollup context to resolve (via any plugins or logic, not only commonjs) with |
|
I took a deep dive into rollups code and found this: https://github.com/rollup/rollup/blob/3cb7f1376f4bbd519d320491629111e1d26cfc80/src/utils/resolveIdViaPlugins.ts#L41 With this information I am certain that rollup does not pass the |
|
yup i've been there a few times during debugging. its wrapping the resolver function in one which uses the accumulated skip set to decide if to skipSelf or not. i was aware of it but assumed that the wrapped resolver being passed to the hook meant though i guess the bit i overlooked was that the hook will call one we can't easily do this pattern though because the context is created per plugin and/or per resolution in our case. nothing easy to wrap. i have tried this route already a few times but got as stuck as this solution. suppose its back to the drawing board anyway, a tomorrow problem. |
|
@daKmoR @thepassle any chance one of you could take a look at this? we think we've solved it (finally). or maybe you could point the right person at it? let me know if you need any help understanding it 👀 |
It is possible a rollup plug injects its own child plugins by hooking into the rollup `options` hook and mutating the options to have extra plugs. This change introduces a check for such plugins and calls them before the host plugin in case they successfully resolve a path first. Similarly, the `resolve` method of plugins receives an options object to hold state for the resolution plugins. We currently lose this object when calling through to `resolveImport`, which can confuse third-party plugins/resolvers heavily. To fix this, `resolveImport` now also accepts a `resolverOptions` so we can pass it on when calling child plugins.
There was a problem hiding this comment.
Is there another instance of a plugin with a child plugin that we could include in the tests in some way that would allow for us to confirm that these changes don't just fix the resolve plugin? Seems like a pretty concise fix, and it would be great if it was broadly applicable beyond the node resolve plugin, while also testing for breakage in other usage so we can hopefully be ahead of issues here in the future.
Once we get that sorted one way or another, we could target this PR to the next branch and get a canary release on merge to confirm again that this works as advertised when leveraging a clean node_modules. I'm pretty sure we've got a couple of projects on the Adobe side that can help test that as well.
If everything goes well up to this point, 
. Doable?
|
@Westbrook i found it difficult to fabricate one in tests which behaves the same way as commonjs/node-resolve, but thats probably what we should do. i have tried this on our largest project - it solves the problems in that case at least, although it raised other issues with using WTR which i need to list at some point i've unfortunately run into a lot of flakiness with cjs support in wtr. even the fact the docs delegate to "just wrap the rollup plugin" without any real focus on how it should work, examples, etc. point to it being incorrectly treated as a non-first-class supported case |
|
I'm really just looking at how to safely get the changes you've proposed here into the library. The semantics of "does WTR support CJS or not" can be argued in a different context. For a "second plugin with child plugins"... I'm not deeply invested in this area, is |
|
its a pretty difficult thing to test @Westbrook you can see the test we added actually relies on the fact that this repo itself has the plugin isn't so much the difficult part here. having a dependency which is structured in a particular way, is. i think to do this test properly/better, we'd need some fixture directory which itself has a im fairly short on time right now too, juggling a big list of tasks/PRs/etc. so if we want to rework the tests, ill need some help. |
|
At this point id argue we should just make a |
|
@43081j is this ready to do that from your stand point? If so, pipit into ready for review and we can get that process moving. |
|
yup i think so. the tests could be better as i mentioned above but im pretty confident the fix is right. |
It is possible a rollup plug injects its own child plugins by hooking into the rollup
optionshook and mutating the options to have extra plugs.This change introduces a check for such plugins and calls them before the host plugin in case they successfully resolve a path first.
Similarly, the
resolvemethod of plugins receives an options object to hold state for the resolution plugins. We currently lose this object when calling through toresolveImport, which can confuse third-party plugins/resolvers heavily.To fix this,
resolveImportnow also accepts aresolverOptionsso we can pass it on when calling child plugins.cc @lucaelin as discussed in #1700
this was an incredibly deep rabbit hole of spaghetti code (outside modernweb), thanks a lot to lucaelin for figuring a bunch of it out too.
its possible we still need to fix rollup upstream as it has some weird import/export mismatch lucaelin is aware of (and already opened an issue for?). weirdly my local copy passes fine now but its possible in the week ive had this up on bricks, i just fixed it in node_modules and forgot about it (to get it working).
WIP for now
as im
pretty surecertain there's still another infinite loop floating around...