Skip to content

chore: drop structuredClone polyfill for v9 #17915

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
Merged
merged 1 commit into from
Dec 29, 2023
Merged

chore: drop structuredClone polyfill for v9 #17915

merged 1 commit into from
Dec 29, 2023

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Dec 29, 2023 •
edited
Loading

I noticed this comment about a thing which could be removed and wanted to make sure it didn't get lost. Happy to close this if you don't want to worry about it.

This doesn't pass linting because no-undef claims that structuredClone is not defined, but I couldn't for the life of me figure out why. The globals package has listed structuredClone in node since 13.12.0. Fixed by upgrade to eslint-plugin-n, see below.

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

Removes a dependency which is no longer necessary in v9.

What changes did you make? (Give an overview)

Removed a polyfill.

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Dec 29, 2023
@netlify Netlify
Copy link

netlify bot commented Dec 29, 2023 •
edited
Loading

✅ Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit b1a9424
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/658e43e974690e0008ee5107

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Dec 29, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: bakkot / name: Kevin Gibbons (b1a9424)

@mdjermanovic
Copy link
Member

This doesn't pass linting because no-undef claims that structuredClone is not defined, but I couldn't for the life of me figure out why. The globals package has listed structuredClone in node since 13.12.0.

@aladdin-add this seems to be related to eslint-plugin-n?

@aladdin-add
Copy link
Member

aladdin-add commented Dec 29, 2023 •
edited
Loading

@aladdin-add this seems to be related to eslint-plugin-n?

Yes, will take a look later!

also, I'm changing the pr title "feat!" => "chore", as it's a not a user-facing change.

@aladdin-add aladdin-add changed the title feat!: drop structuredClone polyfill for v9 feat: drop structuredClone polyfill for v9 Dec 29, 2023
@aladdin-add aladdin-add added chore This change is not user-facing and removed breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Dec 29, 2023
@aladdin-add aladdin-add changed the title feat: drop structuredClone polyfill for v9 chore: drop structuredClone polyfill for v9 Dec 29, 2023
@mdjermanovic
Copy link
Member

In the meantime, I think we can enable the global variable manually in this PR to make the linting pass.

eslint/eslint.config.js

Lines 100 to 102 in 8792464

languageOptions: {
ecmaVersion: "latest"
}, 

languageOptions: {
-    ecmaVersion: "latest",
+    ecmaVersion: "latest",
+    globals: {
+       structuredClone: "readonly"
+    }
},

@aladdin-add aladdin-add reopened this Dec 29, 2023
@mdjermanovic mdjermanovic reopened this Dec 29, 2023
@mdjermanovic
Copy link
Member

Okay, this works now with the new version of eslint-plugin-n, there's no need to update our eslint.config.js.

@mdjermanovic mdjermanovic marked this pull request as ready for review December 29, 2023 12:04
@mdjermanovic mdjermanovic requested a review from a team as a code owner December 29, 2023 12:04
mdjermanovic
mdjermanovic approved these changes Dec 29, 2023
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Would like @nzakas to verify before merging.

nzakas
nzakas approved these changes Dec 29, 2023
View reviewed changes
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@nzakas nzakas merged commit f6f4a45 into eslint:main Dec 29, 2023
@bakkot bakkot deleted the drop-structured-clone-polyfill branch December 29, 2023 18:57
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 27, 2024
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nzakas nzakas nzakas approved these changes

@mdjermanovic mdjermanovic mdjermanovic approved these changes

Assignees

No one assigned

Labels

archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing contributor pool

Projects

Triage
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bakkot @mdjermanovic @aladdin-add @nzakas