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

Remove direct links to Polyfill[dot]io #5127

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

domoscargin
Copy link
Contributor

@domoscargin domoscargin commented Jul 10, 2024

Closes #5105

The included note bulked out the package code considerably (see #dcf4810), so I've gone with a single README in the polyfill folder.

If we wanted to include the note in the code itself, we could add a version of it to src/all.mjs. Then it'd only be included once. But that felt a bit misplaced.

To propagate the changes to the dist and package folders, we'd need to do a patch release. I considered just manually updating these and skipping the CHANGELOG entry, but this seems more transparent, despite being more hassle.

@domoscargin domoscargin linked an issue Jul 10, 2024 that may be closed by this pull request
1 task
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

One little comment/question. This is looking solid otherwise 💪🏻

package/govuk-esm/vendor/polyfills/Date/now.mjs Outdated Show resolved Hide resolved
Removes all comments which link to Polyfill[dot]io or the defunct GitHub repo.

In their place, add a README explaining the situation. Adding a note to each polyfill generates a LOAD of comments in package code, which isn't great.
@domoscargin domoscargin force-pushed the bk-remove-polyfill-links branch from a5eba82 to 059b6bb Compare July 11, 2024 19:50
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-5127 July 11, 2024 19:59 Inactive
@domoscargin
Copy link
Contributor Author

domoscargin commented Jul 11, 2024

@owenatgov I've smooshed ALL related comments now, and added a CHANGELOG entry rather than manually edit the dist and package folders. Just makes everything more obvious, even if it means the hassle of a patch release at some point.

Note for future releasers: the diff to the dist folder was big enough that our validation checks fail because we can't generate a comment that large. We'll have to either disable that comment generation temporarily, or look into increasing its limit somehow?

Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

polyfill.io? Never heard if it.

@domoscargin domoscargin merged commit 2e108a2 into support/4.x Jul 12, 2024
41 checks passed
@domoscargin domoscargin deleted the bk-remove-polyfill-links branch July 12, 2024 20:24
@owenatgov owenatgov mentioned this pull request Oct 10, 2024
@owenatgov owenatgov added this to the 4.9.0 milestone Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace links to polyfill.io in comments on support/v4.x branch
3 participants