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

Framework: Replace element-closest with registered vendor script #9750

Merged
merged 7 commits into from Sep 17, 2018

Conversation

Projects
None yet
5 participants
@aduth
Member

aduth commented Sep 10, 2018

Closes #7159

This pull request seeks to eliminate element-closest dependency and imports from individual packages, instead replacing with a standard polyfill approach via server-side script enqueue.

Potential future improvements include:

  • Improved association between script handle as being a polyfill (avoid wp_add_inline_script per dependent)
  • Avoid polyfill dependencies needing to be kept in sync with current state of codebase using / not using (maybe always load)

Testing instructions:

Verify there are no regressions in Element#closest behaviors, notably in IE11 where it is not supported by default. For example, verify that inserting a quote block and ArrowDown'ing through its citation and into the addition of a default block (verifies usage of Element#closest in WritingFlow's use of isInSameBlock).

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 10, 2018

Contributor

I wonder if we should just do it for wp-polyfill-ecmascript (adding the dependency). This would avoid us to keep track of the exact usage of this feature per script which is very hard to accomplish anyway.

Contributor

youknowriad commented Sep 10, 2018

I wonder if we should just do it for wp-polyfill-ecmascript (adding the dependency). This would avoid us to keep track of the exact usage of this feature per script which is very hard to accomplish anyway.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 10, 2018

Member

@youknowriad Yes, I wasn't too sure how we might want to accomplish the "load always" behavior for element-closest. There's a bit of additional context in #7159 and #7033. With your suggestion, we'd expand what's implied by adding a dependency on wp-polyfill-ecmascript from meaning "latest JavaScript features" to also including "modern browser behaviors". These often collide anyways in our usage, and combining them would make for a simpler developer experience, but they do hold separate meanings which could be worth distinguishing.

Maybe a topic for tomorrow's chat? 🙂

Member

aduth commented Sep 10, 2018

@youknowriad Yes, I wasn't too sure how we might want to accomplish the "load always" behavior for element-closest. There's a bit of additional context in #7159 and #7033. With your suggestion, we'd expand what's implied by adding a dependency on wp-polyfill-ecmascript from meaning "latest JavaScript features" to also including "modern browser behaviors". These often collide anyways in our usage, and combining them would make for a simpler developer experience, but they do hold separate meanings which could be worth distinguishing.

Maybe a topic for tomorrow's chat? 🙂

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 10, 2018

Member

Put another way, ECMAScript and WHATWG/W3 are separate standards, though often used interchangeably as we use modern features and for polyfilling. Should we or shouldn't we abandon attempts to keep them distinct?

Member

aduth commented Sep 10, 2018

Put another way, ECMAScript and WHATWG/W3 are separate standards, though often used interchangeably as we use modern features and for polyfilling. Should we or shouldn't we abandon attempts to keep them distinct?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 10, 2018

Contributor

core-js which is basically what babel-polyfill uses is not only about ECMAScript but it also includes some WHATWG/W3 standards. I'm not really certain if babel-polyfill picks only the ECMAScript features thought or just enqueues the whole thing.

Contributor

youknowriad commented Sep 10, 2018

core-js which is basically what babel-polyfill uses is not only about ECMAScript but it also includes some WHATWG/W3 standards. I'm not really certain if babel-polyfill picks only the ECMAScript features thought or just enqueues the whole thing.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 10, 2018

Member

core-js which is basically what babel-polyfill uses is not only about ECMAScript but it also includes some WHATWG/W3 standards.

Is this true? Could you name some such features?

Genuinely curious because I'd commented as such that my assumption is that it's only for ECMAScript:

#9171 (comment)

Member

aduth commented Sep 10, 2018

core-js which is basically what babel-polyfill uses is not only about ECMAScript but it also includes some WHATWG/W3 standards.

Is this true? Could you name some such features?

Genuinely curious because I'd commented as such that my assumption is that it's only for ECMAScript:

#9171 (comment)

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 10, 2018

Member

Oh I see now from their (core-js) documentation they have a separate section for web standards:

https://github.com/zloirock/core-js#features

Then it becomes a question of: Are we aligned to core-js, or to babel-polyfill? And does babel-polyfill make the same commitments?

Member

aduth commented Sep 10, 2018

Oh I see now from their (core-js) documentation they have a separate section for web standards:

https://github.com/zloirock/core-js#features

Then it becomes a question of: Are we aligned to core-js, or to babel-polyfill? And does babel-polyfill make the same commitments?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 12, 2018

Member

Rebased to account for the now-merged #9794, which simplifies the implementation here a fair bit.

I also discovered that newer versions of JSDOM include support for Element#closest (jsdom/jsdom#1555) such that it wouldn't be necessary to explicitly provide this in jest-preset-default. See 52cff5f.

In updating our pinned version, however, I fought with the license checker flagging abab. I think we've seen this on a few occasions before. Ultimately I decided to add an --ignore option to the license checker, to be used for these sorts of cases where we can manually vet the license for a package as being compatible. The abab package has a compatible license (W3C BSD 3-Clause), but because it is specified in its package.json in a non-parseable format, it is marked as being invalid. See aeb7084.

Member

aduth commented Sep 12, 2018

Rebased to account for the now-merged #9794, which simplifies the implementation here a fair bit.

I also discovered that newer versions of JSDOM include support for Element#closest (jsdom/jsdom#1555) such that it wouldn't be necessary to explicitly provide this in jest-preset-default. See 52cff5f.

In updating our pinned version, however, I fought with the license checker flagging abab. I think we've seen this on a few occasions before. Ultimately I decided to add an --ignore option to the license checker, to be used for these sorts of cases where we can manually vet the license for a package as being compatible. The abab package has a compatible license (W3C BSD 3-Clause), but because it is specified in its package.json in a non-parseable format, it is marked as being invalid. See aeb7084.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 12, 2018

Member

cc @pento re: changes to wp-scripts check-licenses

Member

aduth commented Sep 12, 2018

cc @pento re: changes to wp-scripts check-licenses

@aduth aduth requested a review from pento Sep 12, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 12, 2018

Member

What a can of worms.

Updating jsdom revealed another bug, requiring a testURL value to be specified in our Jest configuration. This shouldn't be necessary (and can therefore be removed) once we update to 23.5.0 or newer.

See: facebook/jest#6766
See: facebook/jest#6792

Member

aduth commented Sep 12, 2018

What a can of worms.

Updating jsdom revealed another bug, requiring a testURL value to be specified in our Jest configuration. This shouldn't be necessary (and can therefore be removed) once we update to 23.5.0 or newer.

See: facebook/jest#6766
See: facebook/jest#6792

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Sep 13, 2018

Member

Licenses are the worst. I mean, I get why they don't put an identifier in the license field, because there isn't a SPDX identifier for the W3C variant of the BSD 3-Clause license. But they could just make it up.

I like the --ignore option, but I hope we never have to use it. 65bf762 adds BSD-3-Clause-W3C as a GPL2 compatible license, and adds a check for if the license field is telling us to look in a license file, instead.

Member

pento commented Sep 13, 2018

Licenses are the worst. I mean, I get why they don't put an identifier in the license field, because there isn't a SPDX identifier for the W3C variant of the BSD 3-Clause license. But they could just make it up.

I like the --ignore option, but I hope we never have to use it. 65bf762 adds BSD-3-Clause-W3C as a GPL2 compatible license, and adds a check for if the license field is telling us to look in a license file, instead.

aduth and others added some commits Sep 12, 2018

Framework: Re-pin to newer version of JSDOM
Newer version is SemVer-compatible with jest-environment-jsdom and includes support for Element#closest out of the box.

jsdom/jsdom#1555

Required to ignore license validation for abab because while it is a compatible license (BSD 3-clause), the package's `license` field is malformed for parse.

https://github.com/jsdom/abab/blob/4327de3aae348710094d9f3c1f0c1477d9feb865/package.json#L26
https://github.com/jsdom/abab/blob/master/LICENSE.md
Testing: Add dummy testURL for Jest config
Avoids issues with localStorage in JSDOM 11.12

Can be removed when running Jest 23.5.0 or newer

See: facebook/jest#6766
See: facebook/jest#6792
Licenses: Add BSD-3-Clause W3C variant as an allowed license
Also, check if the license defined in package.json is telling us to look in the license file, instead.
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 13, 2018

Member

Rebased to resolve conflicts.

At this point just need a 👍

Member

aduth commented Sep 13, 2018

Rebased to resolve conflicts.

At this point just need a 👍

@brandonpayton

I tested this in IE11, and everything appears to work as expected. I also checked this in Chrome and observed that the native version of HTMLElement.prototype.closest was intact.

👍 LGTM

@mcsf

mcsf approved these changes Sep 17, 2018

Good change. :)

@aduth aduth merged commit 45bc8e4 into master Sep 17, 2018

2 checks passed

codecov/project 48.73% (-0.32%) compared to e9d0c47
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the remove/element-closest branch Sep 17, 2018

@aduth aduth added this to the 4.0 milestone Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment