fix: browser resolution in webworkers

[patak-dev]

Description

#2996 and #3039 reworked the package resolution to ignore the browser field if the plugin is running in ssr mode.

See @frandiox comment, if the target is webworker (ie cloudflare workers) then the browser field should be respected even in ssr mode.

This PR adds a targetBrowser param to the resolve methods instead of the previous ssr one. I think that targetBrowser is better here than targetNode to avoid the need to negate it when using it.

const targetBrowser = !(isNode && ssr)

isNode is using the same function from https://github.com/iliakan/detect-node, included in the code to avoid an extra dependency. I don't know if this the most robust way to detect that we are in node and not in a webworker. @frandiox could you check that vite-edge works correctly after this PR (using a dependency that has the browser field, like websocket #2995 or cross-fetch #3036)

This is a regression because #2996 was already included in v2.2.0, #3039 has not yet been released.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[Shinigami92]

@patak-js This just hardly remembered me to this commit from me: mib200/vue-gtm@01f3651#diff-57716bc53661ccdb4571d571f1a5493294848965ae6068dbb3d9a1167d2cc1a6R59

There were some folks that had e.g. issues with electron. Now I provided the user to override the behavior and use their own strategy 🤔
Is this also possible for this situation?

---

But I will approve it now, because if my proposal is helpful, we can implement it in another PR anyway

[patak-dev]

I think we should provide a configuration point if it is not possible to get this info out of the box. We may need it later, but let's grow the API surface when we get the use cases.
I would like to merge this one so we can patch the regression, but I'll wait for @frandiox to check that everything looks good for vite-edge

[frandiox]

@patak-js Thanks so much for this! I've tested with Vite 2.2.1 and it still bundles browser cross-fetch for SSR build. I guess some of the merged PRs are still not released?
If I didn't understand wrong, the next release will default to bundle Node entry point instead of browser, and then this PR adds an optional flag to switch back to browser, right?

I'll try to checkout this PR and test it locally today but basically, if that's the behavior then it should work well :)

Related to this, do you know if there's an equivalent of Rollup's NodeResolve mainFields in Vite?

[patak-dev]

Yes, #3039 has not yet been merged. Thanks for trying out this branch, I'll wait to merge it.
This PR doesn't add a new option, it detects that you are in node+ssr and only for that condition, it ignores the browser field resolution.

About mainFields, there is resolve.mainFields.

[frandiox]

@patak-js Ah I see, thanks for the clarification! In that case this shouldn't be a problem at all. Right now Vitedge uses Node during development. It's only at build time where I need to make sure that browser-entry is used. Since mainFields is exposed (thanks for that) then I can just adapt it when building for web workers.

-- So I've tested this PR and it looks good. As I said, I just need to make sure ssr.noExternal and resolve.mainFields are properly set during SSR build to use browser 👍

Thanks!

[patak-dev]

@frandiox check out the definition for targetBrowser here. You shouldn't need to set resolve.mainFields for this to work in the webworker. Could you test it without that? The idea of this PR is that it the plugins will work out of the box in the webworker during ssr (at least regarding resolution)

[frandiox]

@patak-js The thing is that I use Node.js to build and bundle for webworker, so this is always going to detect Node. That's why I asked if this was a flag that can be set manually. I don't currently have any way to test this running in an actual web worker before building for it :/

Therefore, I need main or module resolution for the dev server (this is the default now) and need to specify browser first when bundling it (it all happens in Node.js, except the production runtime). Unless I'm missing anything.

[patak-dev]

I am closing this PR, at least for the moment, we can assume that we are always in Node when running the plugins in ssr mode. We discussed with @frandiox and what is needed is a target: 'webworker' (webpack uses this naming) if we would like to better support webworker builds.

So, #2996 and #3039 didn't introduce a regression for Node. These PRs should count as a fix, they break plugins that assumed browser resolution in ssr (like vitedge) but that was a bug.