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

use nolyfill to remove some polyfills #31468

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

techknowlogick
Copy link
Member

@techknowlogick techknowlogick commented Jun 24, 2024

We don't need to have polyfills down to Node v4. Some of our deps have polyfills, and don't utilize the built-in implementation if available. While this does decrease our package graph, I haven't been able to notice any decrease/increase in page load times, although that could likely be just because it's already pretty fast.

Nolyfill is https://github.com/SukkaW/nolyfill

updates to files generated with:

npx nolyfill install
npm update

Before this is/isn't merged, I'd be appreciative/thankful for other's insights.

Edit: This isn't due to a specific individual. I am generally supportive of them and their dedication to backward compatibility. This PR is due to not needing those imports for our minimum requirements. Please don't take this PR as commentary on anyone's character.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 24, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 24, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 24, 2024
@silverwind
Copy link
Member

silverwind commented Jun 24, 2024

Could it be ran at the end of make update-js, before the touch? We definitely want this to update whenever dependencies update.

@silverwind
Copy link
Member

silverwind commented Jun 24, 2024

Most if not all of these polyfills come from ljharb's eslint plugins, so I imagine the impact on actual frontend bundle size will be neglible.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 24, 2024
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

See below

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Jun 25, 2024
Comment on lines 891 to +893
npm install --package-lock
npx nolyfill install
npm update
Copy link
Member

@silverwind silverwind Jun 25, 2024

Choose a reason for hiding this comment

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

Suggested change
npm install --package-lock
npx nolyfill install
npm update
npx nolyfill install
npm install --package-lock
  • We need to run it before npm install so that the changes it does take effect
  • We never want to run npm update, we have npx updates to do the same job but better

Copy link
Member Author

Choose a reason for hiding this comment

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

npm update is required because of an upstream bug where npm install doesn't respect the new field in package.json (the cli output shows the link to the bug)

Copy link
Member

@silverwind silverwind Jun 25, 2024

Choose a reason for hiding this comment

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

It links to npm/cli#5850 which says the bug manifests only for non-first installs and first install is when package-lock.json is absent which is the case here as we delete it before. So I don't think my suggestion is affected by that bug.

npm update at this point is pointless, except that is has the danger of upgrading "locked" dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

@silverwind awesome. thanks for looking into that :) I will update with your suggestion above.

Copy link
Member

Choose a reason for hiding this comment

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

To slightly correct myself: "First install" scenario is when both package-lock.json and node_modules are absent which we guarantee above. Reason we do it that way is also to avoid npm/cli#4828. It seems npm has a number of unresolved bugs when not "first install" 😆.

Copy link
Member

@silverwind silverwind Jun 26, 2024

Choose a reason for hiding this comment

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

Actually I will give my suggestion a try because I'm not sure it's ideal to run npx nolyfill with no node_modules present, likely it will work but npx would install it to some temporary location and then install it once more from cache into node_modules. Probably unavoidable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/dependencies modifies/internal size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants