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

Merged
merged 10 commits into from
Jul 24, 2024
Merged

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
Makefile Outdated Show resolved Hide resolved
* origin/main: (59 commits)
  fix OIDC introspection authentication (go-gitea#31632)
  Enable direnv (go-gitea#31672)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  fix redis dep (go-gitea#31662)
  add skip secondary authorization option for public oauth2 clients (go-gitea#31454)
  Fix a branch divergence cache bug (go-gitea#31659)
  [skip ci] Updated translations via Crowdin
  Remove unneccessary uses of `word-break: break-all` (go-gitea#31637)
  [skip ci] Updated translations via Crowdin
  Allow searching issues by ID (go-gitea#31479)
  allow synchronizing user status from OAuth2 login providers (go-gitea#31572)
  Enable `no-jquery/no-class-state` (go-gitea#31639)
  Added default sorting milestones by name (go-gitea#27084)
  Code editor theme enhancements (go-gitea#31629)
  Add option to change mail from user display name (go-gitea#31528)
  Upgrade xorm to v1.3.9 and improve some migrations Sync (go-gitea#29899)
  Issue Templates: add option to have dropdown printed list (go-gitea#31577)
  Fix update flake (go-gitea#31626)
  [skip ci] Updated translations via Crowdin
  ...
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.

I guess the benefits barely outweight the drawbacks here.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Jul 23, 2024
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 23, 2024
@silverwind silverwind enabled auto-merge (squash) July 23, 2024 18:01
@silverwind
Copy link
Member

Latest push had hit npm/cli#4828 again, so I regenerated package-lock.json completely.

@silverwind silverwind merged commit b4ccef3 into go-gitea:main Jul 24, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jul 24, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 24, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 24, 2024
* giteaofficial/main:
  use nolyfill to remove some polyfills (go-gitea#31468)
  Properly filter issue list given no assignees filter (go-gitea#31522)
  Run `detectWebAuthnSupport` only on sign-in page (go-gitea#31676)
  fix OIDC introspection authentication (go-gitea#31632)
  Enable direnv (go-gitea#31672)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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