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

Enable manual polyfill #256

Merged
merged 29 commits into from
Oct 14, 2024
Merged

Enable manual polyfill #256

merged 29 commits into from
Oct 14, 2024

Conversation

marchbox
Copy link
Contributor

@marchbox marchbox commented Oct 8, 2024

Description

This change provides an alternative way for authors to apply polyfill: instead of letting the polyfill to lookup stylesheets and
apply the polyfill, the second argument takes a <style> element and the function can apply polyfill based on its content.

This is particularly useful for components like custom elements, as often the stylesheets are added dynamically to the document, and the currently "auto" polyfilling makes it challenging to apply the changes at the right time.

Related Issue(s)

#228

Copy link

netlify bot commented Oct 8, 2024

Deploy Preview for anchor-position-wpt canceled.

Name Link
🔨 Latest commit b4915fa
🔍 Latest deploy log https://app.netlify.com/sites/anchor-position-wpt/deploys/670d51be6e94ea000870c2cc

Copy link

netlify bot commented Oct 8, 2024

Deploy Preview for anchor-polyfill ready!

Name Link
🔨 Latest commit b4915fa
🔍 Latest deploy log https://app.netlify.com/sites/anchor-polyfill/deploys/670d51beb5ed570008254c4f
😎 Deploy Preview https://deploy-preview-256--anchor-polyfill.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

I'm okay with this approach. I wonder if we want to accept an array of elements, and we could also support <link> tags as well as <style>? Basically using the same functionality as the existing polyfill, but allowing users to opt into a specified list of CSS sources.

@jamesnw @mmalerba What do you think?

src/polyfill.ts Outdated Show resolved Hide resolved
src/polyfill.ts Outdated Show resolved Hide resolved
@jamesnw
Copy link
Contributor

jamesnw commented Oct 9, 2024

I'm okay with this approach. I wonder if we want to accept an array of elements, and we could also support <link> tags as well as <style>? Basically using the same functionality as the existing polyfill, but allowing users to opt into a specified list of CSS sources.

I think an array that supports <link> tags as well as <style> makes a lot of sense.

This does omit inline styles- could we add another boolean flag includeInline that defaults to true, and then adds the response of fetch:fetchInlineStyles()? Or since the inline style impact is scoped to just ones that include anchor, should we just always add the inline styles?

It would be nice to have a demo, but that's a bit of a challenge in the existing single page setup.

@marchbox
Copy link
Contributor Author

marchbox commented Oct 9, 2024

Thanks for the comments. I'll add them in.

I think an array that supports tags as well as <style> makes a lot of sense.

This does omit inline styles- could we add another boolean flag includeInline that defaults to true, and then adds the response of fetch:fetchInlineStyles()? Or since the inline style impact is scoped to just ones that include anchor, should we just always add the inline styles?

Sounds good. I'll pass in the element array into both fetchCSS() and fetchInlineStyle() to build the styleData object.

@marchbox marchbox marked this pull request as ready for review October 9, 2024 23:26
@marchbox
Copy link
Contributor Author

marchbox commented Oct 9, 2024

I updated the code and it's ready for review:

  • Changed the argument of polyfill() to accept an object for configuration, with backward compatibility
  • You can pass in an array of elements for polyfilling, and imperative polyfill uses the same functions as auto polyfill
  • Added demo and tests

Also, maybe I should rename "imperative" to "manual"? Let me know if "manual" sounds better and less confusing.

Copy link
Contributor

@jamesnw jamesnw left a comment

Choose a reason for hiding this comment

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

Excellent work, this is quite helpful! A couple quick notes-

  • It looks like there are a few linting issues, you can run npm run lint to fix or surface.
  • I do prefer manual over imperative slightly, but it's not a blocker.
  • Is it a legitimate use case for people to pass in explicit <style> or <link> elements but want to polyfill all inline styles automatically? If so, that currently isn't possible.
  • If you click the imperative button, and then apply the polyfill to the whole page, the polyfill crashes. This is somewhat related to [BUG] Polyfill fails if a <link> fails to load or returns non-CSS content #255 as well as applying the polyfill to dynamic elements #91, so I'm not concerned here.
  • Do you have any concerns about running this on 2 discrete sets of elements separately? It seems like it should work, as long as there aren't references between the 2 sets of elements.

Overall, thanks for the contribution!

src/polyfill.ts Show resolved Hide resolved
src/fetch.ts Outdated Show resolved Hide resolved
src/fetch.ts Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@marchbox marchbox changed the title Allow polyfilling imperatively for a single style element Enable manual polyfill Oct 10, 2024
@marchbox
Copy link
Contributor Author

marchbox commented Oct 11, 2024

Is it a legitimate use case for people to pass in explicit <style> or elements but want to polyfill all inline styles automatically? If so, that currently isn't possible.

I think that's a valid use case. I'll add this. My only concern is, if the elements already contains some elements that have inline styles, then you pass in includeInlineStyles: false, an author may expect the elements with inline styles in elements not get polyfilled. But this can probably be clarified by documentation. Or maybe we can give includeInlineStyles a different name for more clarity.

Do you have any concerns about running this on 2 discrete sets of elements separately? It seems like it should work, as long as there aren't references between the 2 sets of elements.

Yeah this should work, and this is our intended use since multiple components in our library would need to manual polyfill separately. I also added some test coverage: 912c553.

The only caveat though, is that, if the elements in different sets rely on the same anchor-name, then all polyfill() calls need to include the element that contains this anchor-name declaration. I'm not sure if there's a map somewhere that keeps a record of the anchor name and the generated custom property name. It would be nice to be able to reference an already transformed anchor name.

@marchbox
Copy link
Contributor Author

Added includeInlineStyles: 798561f

@jamesnw
Copy link
Contributor

jamesnw commented Oct 11, 2024

Is it a legitimate use case for people to pass in explicit <style> or elements but want to polyfill all inline styles automatically? If so, that currently isn't possible.

I think that's a valid use case. I'll add this. My only concern is, if the elements already contains some elements that have inline styles, then you pass in includeInlineStyles: false, an author may expect the elements with inline styles in elements not get polyfilled. But this can probably be clarified by documentation. Or maybe we can give includeInlineStyles a different name for more clarity.

I have a slightly different thought on how this could work that may make it more clear (either way, documentation will be useful).

We could call it discoverInlineStyles or transformInlineStyles or ..., have it default to true, and separate it from whether or not elements are passed in. In other words, we default to always finding all the inline styles, but a user can opt out because they are passing in the elements they know have inline styles OR because they know they don't have any inline styles with *anchor.

Would that be simpler for users?

@marchbox
Copy link
Contributor Author

Is it a legitimate use case for people to pass in explicit <style> or elements but want to polyfill all inline styles automatically? If so, that currently isn't possible.

I think that's a valid use case. I'll add this. My only concern is, if the elements already contains some elements that have inline styles, then you pass in includeInlineStyles: false, an author may expect the elements with inline styles in elements not get polyfilled. But this can probably be clarified by documentation. Or maybe we can give includeInlineStyles a different name for more clarity.

I have a slightly different thought on how this could work that may make it more clear (either way, documentation will be useful).

We could call it discoverInlineStyles or transformInlineStyles or ..., have it default to true, and separate it from whether or not elements are passed in. In other words, we default to always finding all the inline styles, but a user can opt out because they are passing in the elements they know have inline styles OR because they know they don't have any inline styles with *anchor.

Would that be simpler for users?

Yeah agreed, this would align better with when there's no argument passed in. But maybe we can name the option as excludeInlineStyles? So when it's not specified, it's falsy.

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

I love this set of changes -- thanks again for the contribution!

Looks like there are two failing tests. I left mostly minor nit comments, and one question/suggestion for expanding the options API.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
src/fetch.ts Outdated Show resolved Hide resolved
src/polyfill.ts Outdated Show resolved Hide resolved
src/polyfill.ts Show resolved Hide resolved
src/polyfill.ts Outdated Show resolved Hide resolved
src/fetch.ts Outdated Show resolved Hide resolved
tests/unit/fetch.test.ts Outdated Show resolved Hide resolved
@jgerigmeyer
Copy link
Member

I'm seeing this too. I think it relates to #253, as the error I see seems to be when the polyfill tries to fetch the already-polyfilled style:

Security Error: Content at http://localhost:3000/ may not load data from blob:http://localhost:3000/8749e98f-f55a-4153-9249-f6bba301e298.

I'm not sure if there's an easy way to read/fetch a blob url, or whether we should explicitly just skip those links?

@marchbox
Copy link
Contributor Author

marchbox commented Oct 12, 2024

I'm seeing this too. I think it relates to #253, as the error I see seems to be when the polyfill tries to fetch the already-polyfilled style:

Security Error: Content at http://localhost:3000/ may not load data from blob:http://localhost:3000/8749e98f-f55a-4153-9249-f6bba301e298.

I'm not sure if there's an easy way to read/fetch a blob url, or whether we should explicitly just skip those links?

Not quite a fix but this would at least let other styles be polyfilled:

try {
  // fetch css and add to array
  const response = await fetch(data.url.toString());
  const css = await response.text();
  return { ...data, css } as StyleData;
} catch {
  return { ...data, css: ''} as StyleData;
}

@marchbox
Copy link
Contributor Author

@jamesnw @jgerigmeyer Thanks for the reviews, both. Have addressed all comments. Ready for another round :)

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

Not quite a fix but this would at least let other styles be polyfilled:

try {
  // fetch css and add to array
  const response = await fetch(data.url.toString());
  const css = await response.text();
  return { ...data, css } as StyleData;
} catch {
  return { ...data, css: ''} as StyleData;
}

I think this might already be fixed by #258? Regardless, I don't think we need to address that in this PR.

Thanks again for this contribution! 🎉

Copy link
Contributor

@jamesnw jamesnw left a comment

Choose a reason for hiding this comment

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

This works great! @marchbox I opened PR #259 to update the docs with the new configuration options- I'd appreciate your eyes on that to make sure it makes sense. Thanks!

@jamesnw jamesnw merged commit c9edaae into oddbird:main Oct 14, 2024
10 checks passed
@marchbox marchbox deleted the add-to-polyfill branch October 14, 2024 18:13
@marchbox
Copy link
Contributor Author

This works great! @marchbox I opened PR #259 to update the docs with the new configuration options- I'd appreciate your eyes on that to make sure it makes sense. Thanks!

Thank you! Will take a look at the documentation changes today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants