Pass along options to resolve

[abeforgit]

closes #2236

There seems to be a convention for rollup plugins which implement the resolveId method to pass along all the options they received in the 3rd argument when calling this.resolve.

The @rollup/commonjs plugin checks for this and throws a warning, stating it might cause some trouble.

This is the full warning:

[plugin commonjs--resolver] It appears a plugin has implemented a "resolveId" hook that uses "this.resolve" without forwarding the third "options" parameter of "resolveId". This is problematic as it can lead to wrong module resolutions especially for the node-resolve plugin and in certain cases cause early exit errors for the commonjs plugin.
In rare cases, this warning can appear if the same file is both imported and required from the same mixed ES/CommonJS module, in which case it can be ignored.

I will admit, the minutiae of this go way over my head, but I can understand the need for making sure plugins can expect any custom stuff they may have added to options to remain there.

It also seemed like it couldn't really break things for the hbs plugin.

This was also suggested by rollup's maintainer here: lightyen/typescript-paths#3 (comment)

uncertainty: lukas suggests spreading the options after the attributes you set yourself, which seems strange to me. I went with the safer approach for embroider. It still fixes the warning in my local testing.

[mansona]

I approved the CI run there so we can see if this passes but I have a few questions/comments

Firstly this is targeting the addon-dev hbs rollup plugin, I can't see any legitimate reason to be using a commonjs resolver plugin in the context of a v2 addon build 🤔 can you talk more about your use case?

The other thing is that this is targeting main, which means it will only be hitting with the unstable versions of the addon-dev package. do you really want this? if you are using the stable versions then you might want to re-target this PR to the stable branch

[abeforgit]

Thank you for your fast reply. I'll answer your questions backwards:
'do you really want this' -> it was only throwing a warning, my build was working fine. My thought process was basically "it seems like some best practices were missed, and it's an easy fix, so let's put in a PR"

"do you really need the commonjs plugin in the first place": I thought I did. I'm importing a very node-based library which I need to ship to the browser. We need a custom webpack config that our apps need to use and everything, to include a bunch of browserify stuff. It's not pretty, but it's what we have atm.
Based on what I could gather online, I thought this implied I needed the commonjs plugin, especially this section of its docs seems to imply so: https://github.com/rollup/plugins/tree/master/packages/commonjs#using-with-rollupplugin-node-resolve

However, I've just tested it and it does appear I don't need it. It's likely something else unrelated was still broken in my v1 to v2 conversion process at the time, which is now fixed.
I find it very hard to learn which piece does what, as most information around build tooling (in general, not embroider) is focused on "here copy-paste this config and you'll be grand, don't worry about why or what it actually does" 😅

In conclusion: this pr is not blocking me, or for that matter anyone else. It does seem to be a good practice to pass along the options, but maybe someone who actually knows what's going on should take it up instead.

[abeforgit]

Hi, I'm back with multiple rabbitholes behind me, and I can now say I officially have a usecase for this PR.

So besides simply playing nice, it is also necessary should an addon ever want to pre-build and bundle a dependency.

For an example of what I mean, have a look at my rollup config https://github.com/lblod/ember-rdfa-editor/pull/1246/files#diff-e89c4fd0b769956d0de73e1dc937c9056af89e0221a3961066f3cff13443d409R37 here

Basically, I have quite a nasty node-entangled dependency that I unfortunately can't live without (yet). While in theory I could let the consuming app take care of the browserification through a webpack config, I ran into some problems with ember 6, which are as of yet still unsolved, but that's besides the point.
I got the idea to fully bundle that dep instead, which would also make consuming my addon easier, since there is now no webpack setup needed. After (a lot) of trial and error, I managed to do this, but only with a patched hbs plugin. (the same patch as this PR)

So what I ended up doing is making sure the embroider plugin didn't handle the dep, and then replacing the import identifier with their browserified variants. This requires the node-resolve plugin and the commonjs plugin to be able to work together, which they do through the use of the options argument to this.resolve.

Now, I admit this will likely remain an incredibly niche usecase, but I really don't see a reason to not follow the recommended practice here.

[abeforgit]

any advice on the CI failure? can I not use the object spread operator maybe?

[mansona]

it looks like the failure is unrelated 😞 since the util tests are checking old node versions we often get this problem where they start failing when someone drops node support incorrectly i.e. not following semver correctly. I think we just need to pin a version of the failing package for these tests and we're good 👍

[mansona]

Hey I just noticed that your PR is targeting main, if you want to get this released soon you will need to target stable since main is tracking the upcoming alpha release.

If you wanted to update your @embroider/addon-dev to point at alpha after we get this merged that's totally fine, but I figured i'd give you the option to decide what you would prefer before I go ahead with this PR