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

Replace uri-js with fast-uri #2415

Merged

Conversation

vixalien
Copy link
Contributor

@vixalien vixalien commented Apr 13, 2024

What issue does this pull request resolve?

This PR fixes issues #2350 and #2343. It removes the (deep) dependency on punycode, a deprecated module.

What changes did you make?

I replaced the default uriResolver to fast-uri instead of uri-js.

Is there anything that requires more attention while reviewing?

This PR supersedes #2377.

It is also worth considering using the web API URL which is defined in URL Living Standard instead of URI which is defined in RFC 3986 and is less prevalent and can resolve the issue raised by @jasoniangreen in the above linked PR:

...I have been speaking to @epoberezkin and he wants me to explore removing uri-js. [..] there is just an open question around browser support. I will update you when I have more.

We can use the native URL API as it is supported with node 10+ and all major browsers and it has 97.68% caniuse score while URI is not standardized afaict.

Thanks!

TODO:

@mbtools
Copy link

mbtools commented Apr 27, 2024

ajv is a major contributor to the [DEP0040] DeprecationWarning: The punycode module is deprecated issue that is popping up everywhere (since it's used in eslint). Please fix this.

Less dependencies. Better performance. This is the way!

@vixalien
Copy link
Contributor Author

I would be very happy if there were an initial review or at least a comment from the AJV team.

@tianyingchun
Copy link

how progress of this?

@tianyingchun
Copy link

@epoberezkin

@tianyingchun
Copy link

how progress of this?

@vixalien
Copy link
Contributor Author

vixalien commented May 9, 2024

No feedback from maintainers, can't move forward :(

@tianyingchun
Copy link

I see that there is still a version of this library released recently,

@vixalien
Copy link
Contributor Author

vixalien commented May 10, 2024

Yes, other pull requests are getting merged and I believe this one is just ignored.

Some feedback - any feedback - from the maintainers would be appreciated

@jasoniangreen
Copy link
Collaborator

Hi all, I will try and get some guidance from @epoberezkin on this one. Sorry I missed your activity here.

@tianyingchun
Copy link

Haha, merge and release a version as soon as possible 😁

@vixalien
Copy link
Contributor Author

Ready

@jasoniangreen
Copy link
Collaborator

Update: After discussing with @epoberezkin, we want to get one more minor release out with some bug fixes and then will release this change as 9.0.0.

@jasoniangreen
Copy link
Collaborator

Update: After discussing with @epoberezkin, we want to get one more minor release out with some bug fixes and then will release this change as 9.0.0.

Actually after discussing with EP and understanding the swap, we've decided this can go out as a minor release. I'm going to give it all one last check and do that next.

Copy link
Collaborator

@jasoniangreen jasoniangreen left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have verified:

  • that uri-js and fast-uri are both built to the same standard RFC 3986
  • that there are test for URNs which was a concern as we have had issues with them in the past
  • that both fast-uri and uri-js are still both being run in tests, we have just swapped what is the default

@jasoniangreen jasoniangreen merged commit fa1b5d5 into ajv-validator:master May 31, 2024
5 checks passed
@jasoniangreen
Copy link
Collaborator

Hmm, something broke the build to master, I think it's related to a bug in the get_contributors script. Investigating.

@jasoniangreen
Copy link
Collaborator

Well I did just publish 8.15.0 with fast-uri but it's failing to build browser bundles because for some reason fast-uri has this line require('node:url')? I have no idea why it's not just require('url') but I think I'll have to roll forward with a revert of this change. Will discuss with @epoberezkin.

jasoniangreen added a commit that referenced this pull request Jun 3, 2024
@yuki-js
Copy link

yuki-js commented Jun 4, 2024

FYI
fast-uri is not officially said browser-incompatible module itself, so it's no wonder it uses node: prefix.

Webpack error message

Module build failed: UnhandledSchemeError: Reading from "node:url" is not handled by plugins (Unhandled scheme).
Webpack supports "data:" and "file:" URIs by default.
You may need an additional plugin to handle "node:" URIs.

kodiakhq bot referenced this pull request in X-oss-byte/Canary-nextjs Jun 4, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [ajv](https://ajv.js.org) ([source](https://github.com/ajv-validator/ajv)) | [`8.14.0` -> `8.15.0`](https://renovatebot.com/diffs/npm/ajv/8.14.0/8.15.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/ajv/8.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/ajv/8.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/ajv/8.14.0/8.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/ajv/8.14.0/8.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>ajv-validator/ajv (ajv)</summary>

### [`v8.15.0`](https://github.com/ajv-validator/ajv/releases/tag/v8.15.0)

[Compare Source](https://github.com/ajv-validator/ajv/compare/v8.14.0...v8.15.0)

#### What's Changed

-   Replace `uri-js` with `fast-uri` by [@&#8203;vixalien](https://github.com/vixalien) in [https://github.com/ajv-validator/ajv/pull/2415](https://github.com/ajv-validator/ajv/pull/2415)
-   Bump to 8.15.0 by [@&#8203;jasoniangreen](https://github.com/jasoniangreen) in [https://github.com/ajv-validator/ajv/pull/2442](https://github.com/ajv-validator/ajv/pull/2442)

#### New Contributors

-   [@&#8203;vixalien](https://github.com/vixalien) made their first contribution in [https://github.com/ajv-validator/ajv/pull/2415](https://github.com/ajv-validator/ajv/pull/2415)

**Full Changelog**: ajv-validator/ajv@v8.14.0...v8.15.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/X-oss-byte/Canary-nextjs).
@voxpelli
Copy link

voxpelli commented Jun 4, 2024

Related: fastify/fast-uri#21

kodiakhq bot referenced this pull request in X-oss-byte/Nextjs Jun 4, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [ajv](https://ajv.js.org) ([source](https://github.com/ajv-validator/ajv)) | [`8.14.0` -> `8.15.0`](https://renovatebot.com/diffs/npm/ajv/8.12.0/8.15.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/ajv/8.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/ajv/8.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/ajv/8.12.0/8.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/ajv/8.12.0/8.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>ajv-validator/ajv (ajv)</summary>

### [`v8.15.0`](https://github.com/ajv-validator/ajv/releases/tag/v8.15.0)

[Compare Source](https://github.com/ajv-validator/ajv/compare/v8.14.0...v8.15.0)

#### What's Changed

-   Replace `uri-js` with `fast-uri` by [@&#8203;vixalien](https://github.com/vixalien) in [https://github.com/ajv-validator/ajv/pull/2415](https://github.com/ajv-validator/ajv/pull/2415)
-   Bump to 8.15.0 by [@&#8203;jasoniangreen](https://github.com/jasoniangreen) in [https://github.com/ajv-validator/ajv/pull/2442](https://github.com/ajv-validator/ajv/pull/2442)

#### New Contributors

-   [@&#8203;vixalien](https://github.com/vixalien) made their first contribution in [https://github.com/ajv-validator/ajv/pull/2415](https://github.com/ajv-validator/ajv/pull/2415)

**Full Changelog**: ajv-validator/ajv@v8.14.0...v8.15.0

### [`v8.14.0`](https://github.com/ajv-validator/ajv/releases/tag/v8.14.0)

[Compare Source](https://github.com/ajv-validator/ajv/compare/v8.13.0...v8.14.0)

#### What's Changed

-   readme: build badge by [@&#8203;epoberezkin](https://github.com/epoberezkin) in [https://github.com/ajv-validator/ajv/pull/2424](https://github.com/ajv-validator/ajv/pull/2424)
-   Update workflows by [@&#8203;rotu](https://github.com/rotu) in [https://github.com/ajv-validator/ajv/pull/2410](https://github.com/ajv-validator/ajv/pull/2410)
-   docs: add warning to maxLength / minLength by [@&#8203;jasoniangreen](https://github.com/jasoniangreen) in [https://github.com/ajv-validator/ajv/pull/2428](https://github.com/ajv-validator/ajv/pull/2428)
-   fix: broken link in docs warning by [@&#8203;jasoniangreen](https://github.com/jasoniangreen) in [https://github.com/ajv-validator/ajv/pull/2431](https://github.com/ajv-validator/ajv/pull/2431)
-   compileAsync a schema with discriminator and $ref, fixes [#&#8203;2427](https://github.com/ajv-validator/ajv/issues/2427)  by [@&#8203;jasoniangreen](https://github.com/jasoniangreen) in [https://github.com/ajv-validator/ajv/pull/2433](https://github.com/ajv-validator/ajv/pull/2433)
-   bump version to 8.14.0 for publishing by [@&#8203;jasoniangreen](https://github.com/jasoniangreen) in [https://github.com/ajv-validator/ajv/pull/2440](https://github.com/ajv-validator/ajv/pull/2440)

#### New Contributors

-   [@&#8203;rotu](https://github.com/rotu) made their first contribution in [https://github.com/ajv-validator/ajv/pull/2410](https://github.com/ajv-validator/ajv/pull/2410)

**Full Changelog**: ajv-validator/ajv@v8.13.0...v8.14.0

### [`v8.13.0`](https://github.com/ajv-validator/ajv/releases/tag/v8.13.0)

[Compare Source](https://github.com/ajv-validator/ajv/compare/v8.12.0...v8.13.0)

-   add named exports
-   update dependencies
-   update node.js

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/X-oss-byte/Nextjs).
@voxpelli voxpelli mentioned this pull request Jun 4, 2024
2 tasks
@fcma-bbientz
Copy link

We are having the same issue with 8.15.0. Since pulling this package our builds have been failing with the following error:

Module build failed: UnhandledSchemeError: Reading from "node:url"

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

Successfully merging this pull request may close these issues.

8 participants