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

Rename wp-polyfill-ecmascript #11216

Merged
merged 8 commits into from
Nov 8, 2018
Merged

Rename wp-polyfill-ecmascript #11216

merged 8 commits into from
Nov 8, 2018

Conversation

earnjam
Copy link
Contributor

@earnjam earnjam commented Oct 29, 2018

We're continuing to see reports of issues with ModSecurity due to a very common ruleset including a rule that blocks files with ecmascript in the filename. The polyfill file uses the name wp-polyfill-ecmascript.min.js, so it ends up getting blocked by the rule, resulting in the editor breaking completely. See #10075 for more details.

It seems unreasonable to expect all hosts to put an exception rule in ahead of 5.0 to make sure this doesn't happen. Any reason why we can't just rename the file?

I went with wp-babel-polyfill on this PR, but not stuck to that. Just used it because that's where the polyfill originates from.

@youknowriad
Copy link
Contributor

At that point, I'd just drop it and add it as a script file for wp-polyfill. Thoughts @aduth?

@aduth
Copy link
Member

aduth commented Oct 29, 2018

At that point, I'd just drop it and add it as a script file for wp-polyfill. Thoughts @aduth?

That's fine with me.

Aside: TIL serious hackers use "ecmascript" in their 'sploits. 🤦‍♂️

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

An issue with this revised approach is that we're referencing the CDN directly, since we're no longer going through the gutenberg_register_vendor_script function to download a local copy of the file. This won't be acceptable for the plugin distributable or the core merge.

Can we keep the gutenberg_register_vendor_script and just rename it to wp-polyfill? Not sure how well this interoperates with the equivalent 5.0 beta script which we're otherwise using gutenberg_override_script to override.

@earnjam
Copy link
Contributor Author

earnjam commented Oct 30, 2018

🤦‍♂️ of course. Sorry was only half paying attention here. I’ll update the PR in a bit

@earnjam
Copy link
Contributor Author

earnjam commented Oct 30, 2018

So I moved it back to using gutenberg_register_vendor_script(), but you're correct in that this would now load the @babel-polyfill script twice if someone is running latest core beta + latest Gutenberg.

We'll need to update this in core in tools/webpack/packages.js as well.

@youknowriad
Copy link
Contributor

@earnjam A simple fix (which is also necessary to ensure backwards compatibility) is to keep wp-polyfill-ecmascript registered but it's content should be null (like the previous wp-polyfill) and it should have wp-polyfill as a dependency.

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Oct 31, 2018
@aduth aduth added this to the 4.3 milestone Oct 31, 2018
'https://cdnjs.cloudflare.com/ajax/libs/babel-polyfill/7.0.0/polyfill' . $suffix . '.js'
);
// Ensure backwards compatibility after renaming to wp-polyfill.
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we want to do away with eventually? Should we make that clearer in the comment? How can we plan to follow-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm honestly not sure best course of action here. Realistically, what is the likelihood that plugins are counting on this script handle? If we plan to eventually remove it, how long should we wait?

Copy link
Member

Choose a reason for hiding this comment

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

We could follow the standard deprecation workflow:

https://wordpress.org/gutenberg/handbook/reference/deprecated/

We've done similar before for server-side features in either (a) enqueuing an inline script to run wp.deprecated( '...' ) messaging or (b) _doing_it_wrong. The former seems easiest, since the second would be difficult in determining whether there are scripts which define a dependency on the script.

Examples:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wp-polyfill-promise looks like a good example of prior art. What version should we set for removal?

Copy link
Member

Choose a reason for hiding this comment

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

Two versions out, 4.5, which may or may not actually happen.

lib/client-assets.php Outdated Show resolved Hide resolved
@aduth
Copy link
Member

aduth commented Oct 31, 2018

To remedy failing tests, you may need to rebase the branch or merge master to account for #11318.

@aduth
Copy link
Member

aduth commented Nov 6, 2018

Added the deprecation warning in 3fc153f

@aduth
Copy link
Member

aduth commented Nov 6, 2018

I dunno who one pings these days as a heads-up for scripts in the 5.0 merge; @pento @atimmer maybe? Proposed here is that we'll not need the wp-polyfill-ecmascript for a final 5.0.

@aduth
Copy link
Member

aduth commented Nov 9, 2018

This inadvertently broke the plugin packaging script. See: #11696

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants