Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Windows bugs by bumping rollup-plugin-polyfill-node to 0.7.0 #3606

Merged
merged 1 commit into from
Aug 3, 2021

Conversation

rpadaki
Copy link
Contributor

@rpadaki rpadaki commented Jul 23, 2021

Changes

Before, when nodePolyfill: true is set in the snowpack config, the files _snowpack/pkg/common/_polyfill-node:XXX.js were not created due to : being an illegal character in Windows filenames. This would exhibit itself as an empty _polyfill-node file being created, and all polyfill imports failing in the application. I fixed this in PR snowpackjs/rollup-plugin-polyfill-node#24 by switching the : to a .

After fixing this issue, I noticed that relative imports in the polyfills -- for example, anything that needed the http-lib polyfills, were failing on Windows. This was because we were using \\ as a path separator in JS imports. I fixed this in PR snowpackjs/rollup-plugin-polyfill-node#26 by using POSIX separators here.

Those PRs were both merged and included in the new release of version 0.7.0 of https://github.com/snowpackjs/rollup-plugin-polyfill-node.

This PR aims to finally:

By bumping to this release of rollup-plugin-polyfill-node!

Testing

I've been using snowpack on Windows for the last few months by manually applying the changes I made to rollup-plugin-polyfill-node, without a hitch. I also made sure to run the tests included in https://github.com/snowpackjs/rollup-plugin-polyfill-node.

For a more detailed testing setup:

  • Set nodePolyfill: true in your snowpack config
  • Run snowpack build and ensure that _snowpack/pkg/common/_polyfill-node.XXX.js file(s) are created and are non-empty
  • Repeat the above for a project that actually uses polyfills and check that they work
  • Repeat the above for a project that uses polyfills from http-lib, and ensure that it actually builds without crashing

Docs

Please let me know if you want me to add documentation, though I think this is just a pretty simple bugfix.

@changeset-bot
Copy link

changeset-bot bot commented Jul 23, 2021

⚠️ No Changeset found

Latest commit: eaf7720

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

@vercel
Copy link

vercel bot commented Jul 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/G4vKSE7tDtnXD2tZTynWbqg5Ag37
✅ Preview: https://snowpack-git-fork-rpadaki-main-pikapkg.vercel.app

Copy link
Collaborator

@drwpow drwpow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing! We’d really love to bump other Rollup deps, but we’re currently limited by bugs in the latest versions of rollup and @rollup/plugin-commonjs (see #2572).

AFAIK, upgrading the node polyfill plugin is safe, or at least, our test suite will prevent any known regressions from happening.

@drwpow drwpow merged commit 3c90b52 into FredKSchott:main Aug 3, 2021
@NfNitLoop
Copy link

@drwpow @rpadaki This hasn't fixed the problem for me.

If I upgrade my snowpack to 3.8.8, it still depends on esinstall ^1.1.7. The 1.1.7 available in npm still depends on

        "rollup-plugin-polyfill-node": "^0.6.2",

I'm not really a Node developer, but I think to fix this for folks someone will need to:

  • Bump the version number of esinstall, and release a new version to npm.
  • Bump Snowpack's dependency on esinstall to require that new version.
  • Publish a new version of snowpack w/ that updated dependency.

Is there a workaround I can use in the meantime?

I tried adding

"rollup-plugin-polyfill-node": "^0.7.0"

to my main package.json, and running an npm install but now I have two versions of it installed instead of upgrading the one esinstall uses. 🤦‍♂️

@NfNitLoop
Copy link

Oh! I see why I can't install 0.7.0. It's actually a major/breaking change from a ^0.6.2 semver. At least according to https://semver.npmjs.com/

@rpadaki
Copy link
Contributor Author

rpadaki commented Sep 13, 2021

@drwpow @rpadaki This hasn't fixed the problem for me.

If I upgrade my snowpack to 3.8.8, it still depends on esinstall ^1.1.7. The 1.1.7 available in npm still depends on

        "rollup-plugin-polyfill-node": "^0.6.2",

I'm not really a Node developer, but I think to fix this for folks someone will need to:

  • Bump the version number of esinstall, and release a new version to npm.
  • Bump Snowpack's dependency on esinstall to require that new version.
  • Publish a new version of snowpack w/ that updated dependency.

Is there a workaround I can use in the meantime?

I tried adding

"rollup-plugin-polyfill-node": "^0.7.0"

to my main package.json, and running an npm install but now I have two versions of it installed instead of upgrading the one esinstall uses. 🤦‍♂️

Hey @NfNitLoop! I'm not really a node dev either, but I'm happy to help you figure out a workaround at the very least.

Is there a particular repo/project I should try running? Is there a slack/discord/irc/etc where it would be convenient to chat?

@Alfagun74
Copy link

I have this issue aswell with socket.io-client 4.2.0 and Snowpack 3.8.8

@impeas
Copy link

impeas commented Sep 24, 2021

I have the same issue aswell

@lmalsch
Copy link

lmalsch commented Oct 5, 2021

Same issue here...

@leruaa
Copy link

leruaa commented Oct 26, 2021

Hello @rpadaki,

Looks like we just need to increment the esinstall version in esinstall/package.json, deploy it to npm, and use the new version in Snowpack, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants