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

feat: implement Set.isSubsetOf() for Node <22, which defaults to builtin implementation whenever possible #1009

Merged
merged 8 commits into from
Nov 7, 2024

Conversation

novusnota
Copy link
Member

Issue

Closes #1008.

Checklist

  • I have updated CHANGELOG.md
  • I have run all the tests locally and no test failure was reported
  • I have run the linter, formatter and spellchecker
  • I did not do unrelated and/or undiscussed refactorings

@novusnota novusnota added this to the v1.6.0 milestone Oct 29, 2024
@novusnota novusnota requested a review from a team as a code owner October 29, 2024 15:37
Copy link
Contributor

@verytactical verytactical left a comment

Choose a reason for hiding this comment

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

Please don't patch any prototypes. Other libraries might expect builtin ES methods to work exactly to the letter of specification, and this implementation doesn't really look like it.

In case something goes wrong, we'll be up to hours of debug, because prototype patching changes behavior of other code remotely and without any trace.

It would be much easier both for implementer and for future users to just define isSubsetOf function, and optionally default it to builtin .isSubsetOf method for increased performance on newer JS engines.

@verytactical
Copy link
Contributor

verytactical commented Oct 29, 2024

I should also mention that the concept of "polyfill" relies on the process around it. Polyfills are supplied by well-tested libraries, their authors are well-versed in spec legalese, and, most importantly, polyfills are applied by build infrastructure (for example, babel). Due to wider adoption of those polyfills, we reduce detection time for inevitable bugs in their implementations, and get centralized updates and security audits.

In this context neither the word "polyfill" nor its implementation appears as something end user might encounter in production codebase. At least not under regular circumstances.

@novusnota
Copy link
Member Author

novusnota commented Oct 29, 2024

@verytactical I generally agree against prototype patching, but thought this particular case might be fine. Plus, I simply took the working polyfill from Web IDE repo. Which, I suppose, was in turn taken from core-js. Or not :)

Anyways, I can re-write one or another implementation as a global function with optional fallback to the builtin method if/when it's present

@novusnota novusnota changed the title feat: add .isSubsetOf() polyfill feat: implement Set.isSubsetOf() for Node <22, which defaults to builtin implementation whenever possible Oct 29, 2024
@verytactical
Copy link
Contributor

Please, define it as a function.

In case of WebIDE it's apparently either used in one of the libraries, or not used at all. Probably it's there, because it compiles code with tact, and with this PR it won't be needed anymore. Unfortunately I don't know circumstances of when it was added.

cc @rahulyadav-57

@novusnota novusnota force-pushed the closes-1008-isSubsetOf-polyfill branch from 47ed53c to 84f7082 Compare October 31, 2024 14:48
@novusnota
Copy link
Member Author

novusnota commented Oct 31, 2024

@verytactical all good now — the function is global, but your introduction of fs.glob in scripts/copy-files.ts has tied us to Node.js 22 :)

UPD: Turns out we do have the glob package used elsewhere, so I just replaced fs.glob() with glob.sync()

@novusnota novusnota force-pushed the closes-1008-isSubsetOf-polyfill branch from 0cfc84c to 80f4edc Compare November 6, 2024 15:27
verytactical
verytactical previously approved these changes Nov 6, 2024
Copy link
Contributor

@verytactical verytactical left a comment

Choose a reason for hiding this comment

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

I think I should stop bikeshedding this :)

scripts/copy-files.ts Show resolved Hide resolved
src/utils/isSubsetOf.ts Show resolved Hide resolved
src/utils/isSubsetOf.ts Outdated Show resolved Hide resolved
src/utils/isSubsetOf.spec.ts Outdated Show resolved Hide resolved
src/utils/isSubsetOf.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@verytactical verytactical left a comment

Choose a reason for hiding this comment

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

.

@verytactical verytactical merged commit b8c07e1 into main Nov 7, 2024
17 checks passed
@verytactical verytactical deleted the closes-1008-isSubsetOf-polyfill branch November 7, 2024 12:07
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.

Add a polyfill or a replacement for .isSubsetOf() to support Node.js <22
2 participants