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

ensure node support since node 10 #119

Merged
merged 6 commits into from
Jan 6, 2025
Merged

ensure node support since node 10 #119

merged 6 commits into from
Jan 6, 2025

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jan 6, 2025

Checklist

@Fdawgs
Copy link
Member

Fdawgs commented Jan 6, 2025

But why? Node 10 became EOL almost 4 years ago, and Node 14 nearly 2 years ago.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 6, 2025

Unfortunately I think we need to keep up the contract with ajv, which supports node 10 till latest. :/

@Fdawgs
Copy link
Member

Fdawgs commented Jan 6, 2025

From looking at the ajv repo, they don't have an engines value in the package.json, and they only test from node 16 onwards in their GitHub Workflows.

No mention of a minimum Node version on the ajv site either 😬

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 6, 2025

image

@Fdawgs
Copy link
Member

Fdawgs commented Jan 6, 2025

image

God damn it. Going to make a PR over there then for them to test on node 10+.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 6, 2025

@Fdawgs
Maybe they removed it with purpose? Idk. :/

ajv-validator/ajv@43d6164

@Fdawgs
Copy link
Member

Fdawgs commented Jan 6, 2025

@Fdawgs Maybe they removed it with purpose? Idk. :/

ajv-validator/ajv@43d6164

You're right, Ajv won't even build with Node 10: https://github.com/ajv-validator/ajv/actions/runs/12629749595/job/35188245130?pr=2522

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 6, 2025

Maybe we should ajk @epoberezkin to clarify the docs and remove node 10 - 14 as supported environments?

@Fdawgs
Copy link
Member

Fdawgs commented Jan 6, 2025

Maybe we should ajk @epoberezkin to clarify the docs and remove node 10 - 14 as supported environments?

ajv-validator/ajv#2523 :)

@climba03003
Copy link
Member

To be fair, ajv is using TypeScript and targeting to ES2018 which can be run on node@10.
The CI build failure only tells it cannot be built on old system but it doesn't shows it can't run on old system.

The proper CI should be built on fixed version (maybe 22) first, then run the test on the supported system.

@Fdawgs
Copy link
Member

Fdawgs commented Jan 6, 2025

Thanks @climba03003, so i guess best thing to do would be to revert the optional chaining and add Node 10 - 14 to the CI?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 6, 2025

@Fdawgs
@climba03003

I think I made the proper changes, to fix it here. How should we progress?

I basically fix also the transient fast-uri dependency of ajv to ensure that it really works.

@climba03003
Copy link
Member

Yes, it also affect fastify@4 users.
We can land the optional chaining and bump a major.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 6, 2025

@climba03003

I dont think that we should bump a major here and make it depending on the versioning of fastify. This is a library and not a plugin of fastify.

@Fdawgs Fdawgs merged commit 1d4278a into main Jan 6, 2025
23 checks passed
@Fdawgs Fdawgs deleted the support-since-node-10 branch January 6, 2025 10:07
@Fdawgs
Copy link
Member

Fdawgs commented Jan 6, 2025

I'll do a quick release, thanks @Uzlopak

@climba03003
Copy link
Member

I dont think that we should bump a major here and make it depending on the versioning of fastify. This is a library and not a plugin of fastify.

The question is actually simple, are we continue to support node@10 based on the downstream package (ajv)?
If yes, then remain the same.
If no, we must break some downstream again in order to use latest feature.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 6, 2025

@Fdawgs
@climba03003

We should maybe discuss this in contributor discussions, because we have other libraries in our org. Probably we need to decide if specific dependencies have "api contracts" with other projects. Here we have the special case, that we provided fast-uri as replacement of urljs to ajv and basically bound our library to ajv in a strong bound. Maybe we need to document this strong bound in the readme md. Like "this library works with ajv v8" and thus makes it mandatory to support the same range of engines.

Maybe @epoberezkin clarifies, that node 10 is not supported anymore, but this could result in alot of changes in upstream projects. I see issues in angular and stuff.

To be honest, I dont like the idea, that we have to carry projects, were node 12 is used to build the project. The maintainer should then switch to latest node. But this does not answer the question if ajv is purposefully supporting node 10+. Lets see what happens.

parsed.path = escape(unescape(parsed.path))
}
if (parsed.fragment?.length) {
if (parsed.fragment && parsed.fragment.length) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit but parsed.fragment would also cover the length check here if a string indeed exists

Copy link
Member

Choose a reason for hiding this comment

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

@gurgunday Fancy throwing a PR together?

@gurgunday gurgunday mentioned this pull request Jan 6, 2025
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.

4 participants