bug: server only built-ins polyfilled in client bundle

[p-m-p]

Hi, I've been unable to update @azure/msal-node library since version 1.12.1 due to the introduction of the loopback listener which imports createServer from the http builtin as per below.

import http, { createServer } from 'http';

From what I can tell from the error message it is trying to resolve from the rollup http polyfill plugin that doesn't implement this method. I tried adding the library to serverDependenciesToBundle but that didn't seem to solve anything and if I edit the imported file in node_modules to just import http from 'http'; everything builds and runs as expected.

Appreciate any help or direction to getting this resolved. Thanks.

[changeset-bot]

⚠️ No Changeset found

Latest commit: da8d178

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[remix-cla-bot]

Hi @p-m-p,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[kiliman]

if I edit the imported file in node_modules to just import http from 'http'; everything builds and runs as expected.

Are you editing the @azure/msal-node package? If so, you can use patch-package to ensure your fix will continue.

[p-m-p]

if I edit the imported file in node_modules to just import http from 'http'; everything builds and runs as expected.

Are you editing the @azure/msal-node package? If so, you can use patch-package to ensure your fix will continue.

Only to verify what I thought to be the issue. I might consider that as a temporary solution though, thanks. I'd still like to get to the bottom of the issue, if I try a similar import in the application code there is no issue so seem to only be from third party libraries.

[p-m-p]

I spent some time looking into this and have resolved the initial error by renaming the imported file with the suffix .server.ts so that it is treated as an empty module in the client bundle. Other than the entry.server.ts file this feature seems to not be clearly documented but more loosely referred to as a convention, I'd be happy to update the docs somewhere to make this more clear if desired (ref https://github.com/remix-run/remix/blob/main/packages/remix-dev/compiler.ts#L353).

If you do import from a built-in or a module that depends on one to use in server only code in a route and the polyfill does not implement it then you'll see an error like with createServer from http in this instance. If it is fully implemented and no error occurs then the polyfill code ends up in the browser bundle and is not fully eliminated. I've updated the bug test in this PR to demonstrate this.

I see there is an open pull request on the plugins package for some time, perhaps bringing this change in as a maintained plugin would be a better choice? I could potentially help out here now I am familiar with the code but perhaps this issue raises a question if use of the plugins should actually be removed instead?

[MichaelDeBoey]

Closed by #5274

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-987ee78-20230414 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[jacob-ebey]

This lib is not meant to be utilized in the browser and is expected to require / use a bunch of node polyfills.

[p-m-p]

The issue I faced here was with using from a loader in a route which perhaps is considered client code but that wasn't clear to me. I did solve it as per my previous comment by renaming the included file with .server.ts to it wasn't bundled but left this here due to the issue with the polyfills that looks to be closed now so is likely no longer an issue.