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

Add no-unnecessary-polyfills rule #1717

Merged
merged 4 commits into from
Nov 2, 2023

Conversation

Mesteery
Copy link
Contributor

@Mesteery Mesteery commented Feb 5, 2022

Fixes: #36


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


@zloirock
Copy link
Contributor

zloirock commented Feb 6, 2022

For core-js@3 / core-js-pure, use core-js-compat - @babel/compat-data is not the proper data source for them.

@Mesteery
Copy link
Contributor Author

Is this what you suggested? @zloirock

@zloirock
Copy link
Contributor

@Mesteery for core-js - yes, but not for the rest polyfills. I meant that you could use entries for the rest polyfills too. For example, you can get the list of features like

Object.keys(coreJsEntries).flatMap(it => (it.startsWith('core-js/actual/') && !/\/(virtual|instance)\//.test(it)) ? it.slice(15) : [])

and operate with them like with core-js entries.

@Mesteery
Copy link
Contributor Author

Sorry, but I didn't quite understand that.

@zloirock
Copy link
Contributor

core-js modules do not directly correspond to language features. Entry points - correspond. Many language features depend on some modules, like with Set example above. A module is an abstract part of a language feature polyfill.

For example, you have an import of string-at-polyfill. With your regex patterns, you can understand that it's an at method of String. However, core-js module for this feature is es.string.at-alternative - you can't directly rely on it. However, since you know that it's an at method of String, you can rely on core-js/actual/string/at data and you will see that this feature depends on es.string.at-alternative module - and check compact data for this module. If it depends on some modules - you can check all of them, like now it's done in the core-js case.

@Mesteery
Copy link
Contributor Author

Like this 707f4f2?

@zloirock
Copy link
Contributor

Yes, something like that.

@Mesteery
Copy link
Contributor Author

Mesteery commented Mar 6, 2022

PTAL @fisker

@Mesteery Mesteery requested a review from fisker July 18, 2022 22:31
@fisker
Copy link
Collaborator

fisker commented Jul 19, 2022

Sorry for the delay, I'll review ASAP.

Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay. Feel hard to review, I'll take another round later.

}

const {browserlist} = result.pkg;
const nodeEngine = result.pkg.engines && result.pkg.engines.node && new SemverNodeVersion(result.pkg.engines.node);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not every package.json have engine field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that is in case the targets field is not defined in the config. If the engines field is also undefined, the plugin is not executed.

rules/no-unnecessary-polyfills.js Outdated Show resolved Hide resolved
rules/no-unnecessary-polyfills.js Outdated Show resolved Hide resolved
test/no-unnecessary-polyfills.mjs Outdated Show resolved Hide resolved
docs/rules/no-unnecessary-polyfills.md Outdated Show resolved Hide resolved
rules/no-unnecessary-polyfills.js Outdated Show resolved Hide resolved
docs/rules/no-unnecessary-polyfills.md Outdated Show resolved Hide resolved
rules/no-unnecessary-polyfills.js Outdated Show resolved Hide resolved
test/no-unnecessary-polyfills.mjs Outdated Show resolved Hide resolved
@Mesteery Mesteery requested a review from fisker September 1, 2022 18:20
@fisker
Copy link
Collaborator

fisker commented Sep 4, 2022

The tests are failing.

@Mesteery
Copy link
Contributor Author

Mesteery commented Sep 5, 2022

Fixed. The errors is due to STATIC_REQUIRE_SOURCE_SELECTOR (maybe i misused it, but it worked correctly with STATIC_REQUIRE_SELECTOR), and the Object.assign (. instead of #, in tests), there is no way to know if it is a static method or not... except hardcoding static methods.

@Mesteery
Copy link
Contributor Author

Mesteery commented Sep 7, 2022

Thanks you! Is it ready or i missed some changes?

@sindresorhus
Copy link
Owner

@Mesteery Bump

@Mesteery
Copy link
Contributor Author

Mesteery commented Feb 5, 2023

I'm sorry for all the delay, I was really busy. (btw, I don't know how to fix the markdown lint error)

I have a doubt about one thing, I'm not sure if corejs({targets}) supports SemVer syntax with targets = {node:'semver'}

@fisker
Copy link
Collaborator

fisker commented Feb 6, 2023

I'm not sure if corejs({targets}) supports SemVer syntax with targets = {node:'semver'}

#1717 (comment)

@Mesteery
Copy link
Contributor Author

If I understand you want node.engines to be core-js targets?

@sindresorhus
Copy link
Owner

Bump

@fisker
Copy link
Collaborator

fisker commented Jun 20, 2023

I mean I don't know if core-js accept engines if not, translate to targets, other user input targets pass directly to core-js.

@Mesteery
Copy link
Contributor Author

Mesteery commented Jul 1, 2023

I apologize again for the delay. I've slightly refactored the code, and I passed engines directly to corejs targets if browserlist is not defined (and I've removed useBrowserlistFieldByDefault, as I think it would make more sense to always have engines as a last resort). However, I'm not sure that corejs correctly supports the semver syntax that can be used in engines.

@Mesteery Mesteery requested a review from fisker September 9, 2023 19:55
docs/rules/no-unnecessary-polyfills.md Outdated Show resolved Hide resolved
docs/rules/no-unnecessary-polyfills.md Outdated Show resolved Hide resolved
rules/no-unnecessary-polyfills.js Outdated Show resolved Hide resolved
rules/no-unnecessary-polyfills.js Outdated Show resolved Hide resolved
rules/no-unnecessary-polyfills.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

CI is failing

@Mesteery Mesteery force-pushed the no-polyfill-rule branch 2 times, most recently from bdc43eb to 3c7610d Compare October 30, 2023 22:28
rules/no-unnecessary-polyfills.js Outdated Show resolved Hide resolved
rules/no-unnecessary-polyfills.js Outdated Show resolved Hide resolved
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@sindresorhus sindresorhus merged commit 6788d86 into sindresorhus:main Nov 2, 2023
21 checks passed
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.

Rule proposal: no-unnecessary-polyfills
4 participants