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.

Sign up for GitHub

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

Jump to bottom

Remove patched dom types (v6.x branch) #3531

Merged
merged 2 commits into from
Sep 2, 2024
Merged

Remove patched dom types (v6.x branch) #3531

merged 2 commits into from
Sep 2, 2024

Conversation

eXhumer
Copy link
Contributor

@eXhumer eXhumer commented Sep 1, 2024 •
edited
Loading

This relates to...

Fixes #3524

Rationale

Several missing DOM types from @types/node were added as part of patch.d.ts. Event & EventTarget exported DOM types in global were overwritten with patch.d.ts.

Changes

  • Remove patched Event & EventTarget type and references to it from other declaration files.
  • Lock version for @types/node to ~18.17.19.

Features

N/A

Bug Fixes

  • Fixes typing bug related to DOM Event type.

Breaking Changes and Deprecations

N/A

Status

Notes

This only applies to branch v6.x, which is the latest stable version available currently. This will also need to be ported for the main branch, where the major version was bumped to v7.

@types/node version was updated to reflect the minimum supported engine version in package.json.

@eXhumer
fix: Remove patched DOM types Event & EventTarget
cfdeac5 
* see #3524

Signed-off-by: eXhumer <exhumer@exhumer.cc>
@eXhumer
better @types/node version lock
3701c06 
* Add minimum types for node based on `engines` version
* Currently undici locks engine to node@>=18.17
* Get @types/node for 18.17.x specificially and lock to patch upgrades only for 18.17

Signed-off-by: eXhumer <exhumer@exhumer.cc>
mcollina
mcollina approved these changes Sep 1, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

package.json
@@ -105,7 +105,7 @@
"@fastify/busboy": "2.1.1",
"@matteo.collina/tspl": "^0.1.1",
"@sinonjs/fake-timers": "^11.1.0",
"@types/node": "^18.0.3",
"@types/node": "~18.17.19",
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

@eXhumer eXhumer Sep 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This just provides type for node 18.17 specifically, which is the minimum required based on the engines version. The listed version ^18.0.3 doesn't contain the Event and EventTarget. And the version that ends up being installed installs typing related to node 18.19 (18.19.47) instead of the minimum supported 18.17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert back this change if necessary, but I strongly recommend locking the @types/node to the minimum supported version mentioned in engines.node in package.json.

@eXhumer
Copy link
Contributor Author

eXhumer commented Sep 1, 2024

Should I create a separate PR for the main branch or should I close this PR and create a PR targeting the main branch and then back port the patch with backport.yml workflow?

@mcollina
Copy link
Member

mcollina commented Sep 1, 2024

If possible target the main branch and we use the backport action.

@eXhumer eXhumer closed this Sep 1, 2024
@eXhumer eXhumer mentioned this pull request Sep 1, 2024
Merged
5 tasks
@mcollina mcollina reopened this Sep 2, 2024
@mcollina
Copy link
Member

mcollina commented Sep 2, 2024

@eXhumer PTAL, I think we'd need a manual backport after all.

@eXhumer
Copy link
Contributor Author

eXhumer commented Sep 2, 2024

Correct. This requires manual backport to v6.x as File API and related APIs get replaced for node's buffer module for main branch (in development for v7). In v6.x, DOMException is used within types/filereader.d.ts. Also, in v6.x branch, we have to replace the patched Event & EventTarget within types/filereader.d.ts as well.

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 2, 2024

I guess #3533 must be first merged before merging this.

@mcollina
Copy link
Member

mcollina commented Sep 2, 2024

@eXhumer @Uzlopak is this ready to land?

@eXhumer
Copy link
Contributor Author

eXhumer commented Sep 2, 2024

Yes, LGTM

@mcollina mcollina merged commit 8d28413 into nodejs:v6.x Sep 2, 2024
55 of 56 checks passed
@eXhumer eXhumer deleted the remove-patched-dom-types-v6.x branch September 2, 2024 19:26
@leonardoadame leonardoadame mentioned this pull request Oct 31, 2024
Open
@dolevmiz1 dolevmiz1 mentioned this pull request Nov 2, 2024
Open
@thoukydides thoukydides mentioned this pull request Nov 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eXhumer @mcollina @Uzlopak