fix: regression in browser objects pointing to nested node_mpodules

[keithamus]

This fixes an issue mentioned by @paulsouche in this comment: #136 (comment)

@paulsouche would you be so kind as to test this with your build to ensure it still passes?

[keithamus]

Very confused that these pass on my machine but not on CI.

[paulsouche]

Hi,

This fixes the issue with my build, but I have same issue locally with test as on CI. require is not defined

Can't wait for this !

Thank you

[bterlson]

@guybedford @TrySound @lukastaegert @keithamus can I help land this soon?

With rollup-plugin-node-builtins throwing audit warnings, I'm trying to roll my own package replacements, and it's difficult to do without being able to alias to node-modules. This would also enable many more packages json with browser fields to "just work" with rollup. Let me know if I can be of help!

[lukastaegert]

I can have a look tomorrow

[lukastaegert]

Ok, so as I understand it: The issue was that even though the browser field correctly resolved to another node module, this module did not exist. Consequently the bundle contained a dangling require statement that could not be resolved by the test harness.

I fixed this by adding an appropriate module (my guess is that this existed locally but was not committed due to some global .gitignore or similar). I also updated the branch to latest master and update the dependencies. I also changed the tests to explicitly list all warnings that are expected to occur.

There is one thing that does not appear ideal to me but probably would require a change in rollup itself and this is how the case is handled where the node module that the browser field points to is missing. In this case, not the replaced import but the original import remains in the bundle. To improve this, however, Rollup would need a mechanism for plugins to identify an import as missing while changing the importee (Currently, you can just return false which does not change the id. If you return something that does not resolve to an existing file, Rollup will throw).

From my side, this is good to be released.

[lukastaegert]

Released