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(pnp): avoid using global URL #6030

Closed
wants to merge 10 commits into from
Closed

Conversation

llowrey
Copy link

@llowrey llowrey commented Dec 21, 2023

  • Added URL to import from 'url' package
  • Necessary as fileURLToPath() will reject if not URL from 'url' package

What's the problem this PR addresses?
This addresses issue #5915. Some dependency, appears to have been react in my case, and under certain circumstances (may be specific to vitest, but maybe not), causes a call to new URL() which is rejected by a subsequent call to fileURLToPath() which, for node version < 20, will reject URL instances that aren't instances of the url package URL class.

This PR is able to easily fix the problem by including URL in the imports from the url package.

Resolves #5915
...

How did you fix it?

By adding URL to the import statements of a couple of yarnpkg-pnp files, the generated .pnp.cjs file will instantiate URLs via new url.URL() so that the resulting objects are acceptable by fileURLToPath().

...

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

* Added URL to import from 'url' package
* Necessary as fileURLToPath() will reject if not URL from 'url' package
@llowrey llowrey marked this pull request as ready for review December 21, 2023 02:43
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

This seems like something that should be fixed in Vitest.

packages/yarnpkg-pnp/package.json Outdated Show resolved Hide resolved
packages/yarnpkg-cli/package.json Outdated Show resolved Hide resolved
@llowrey
Copy link
Author

llowrey commented Dec 21, 2023

This seems like something that should be fixed in Vitest.

The problem I found while debugging within .pnp.cjs was that the URL instances it was creating were empty objects (console.log output was {}) but serialized to valid URLs (via .toString() or JSON.stringify()). My guess is that at some point in the bootup process, vitest, or some transitive dependency, is replacing global.URL with an implementation that is meant to emulate the in-browser URL class.

If that's true, perhaps vitest should be doing that later in the bootup process but I don't know if that is possible.

Regardless, this change hardens yarn. It is also not unprecedented as there are many other files in the project which import URL explicitly.

@merceyz
Copy link
Member

merceyz commented Dec 21, 2023

Regardless, this change hardens yarn. It is also not unprecedented as there are many other files in the project which import URL explicitly.

That's fair I suppose, I've updated .eslintrc.js to prevent this from happening again and fixed the rest of them.

@merceyz merceyz changed the title Fixed pnp use of wrong URL class fix(pnp): avoid using global URL Dec 21, 2023
@arcanis
Copy link
Member

arcanis commented Jan 3, 2024

It seems there's a legit issue with building the website:

11:52:05 AM: [success] [webpackbar] Client: Compiled with some errors in 25.06s
11:52:05 AM: Module not found: Error: Can"t resolve "url" in "/opt/build/repo/packages/plugin-git/sources"
BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
11:52:05 AM: This is no longer the case. Verify if you need this module and configure a polyfill for it.
11:52:05 AM: If you want to include a polyfill, you need to:
11:52:05 AM: 	- add a fallback "resolve.fallback: { "url": require.resolve("url/") }"
11:52:05 AM: 	- install "url"
11:52:05 AM: If you don"t want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "url": false }

arcanis pushed a commit that referenced this pull request Jan 15, 2024
**What's the problem this PR addresses?**

Before nodejs/node#46904 using a custom global
URL class wasn't supported by `fileURLToPath`.

Closes #6030
Fixes #5915

**How did you fix it?**

- Created a rollup plugin to ensure that the builtin URL class is always
used on Node.js < v20.
- Removed all imports of `URL` since it's a global and to avoid mixing
custom and builtin URL class instances.

**Checklist**
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).
- [x] I have set the packages that need to be released for my changes to
be effective.
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
@arcanis
Copy link
Member

arcanis commented Jan 15, 2024

Thanks @llowrey !

billhimmelsbach added a commit to cfpb/sbl-frontend that referenced this pull request Apr 30, 2024
Found this bug when trying to run tests locally:

<img width="961" alt="Screenshot 2024-04-27 at 4 43 24 PM"
src="https://github.com/cfpb/sbl-frontend/assets/19983248/669e9a58-a713-435c-a5b9-0c131b46938d">

This bug is [caused by](yarnpkg/berry#6030)
running on an old version of node on older versions of yarn when using
vitest, which has [been fixed in later
updates](yarnpkg/berry#6030). Updating to the
latest version of yarn fixes this issue (the [DS did it earlier this
year](cfpb/design-system@2691a16)),
but v4 of yarn also requires at least node 18. We should at least update
from node 16 to node 18 anyway for the engine, [since it's at
EOL](https://github.com/nodejs/Release). We could also update all the
way to node 20, or even 22 if we're ok with it not being a LTS release
for a few months. We can probably wait till post-mvp for that change.

## Changes
- update yarn to v4
- update node to 18, maybe even 20/22 post-mvp if we're ok not being on
a LTS release for a few months

## How to test this PR

- make sure you're using node 18, if you're using nvm, it's `nvm use 18`
- make sure you're using the prescribed version of yarn, it's `yarn set
version 4.1.1` or use corepack
- does vite still build things correctly? `yarn run dev`
- can you run your tests locally? `yarn test`

## Screenshots

### Before

<img width="961" alt="Screenshot 2024-04-27 at 4 43 24 PM"
src="https://github.com/cfpb/sbl-frontend/assets/19983248/1f0c7dcf-9419-4961-a6d6-8b5754fc8300">

### After
![Screenshot 2024-04-28 at 9 34 36
AM](https://github.com/cfpb/sbl-frontend/assets/19983248/d9078f72-1586-4170-a738-b3030da9816a)

---------

Co-authored-by: Meis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants