Make WordPress Core

Opened 3 years ago

Closed 16 months ago

Last modified 15 months ago

#52851 closed enhancement (fixed)

Update the `element-closest` polyfill

Reported by: desrosj's profile desrosj Owned by: desrosj's profile desrosj
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: needs-testing has-patch add-to-field-guide
Focuses: Cc:

Description

The element-closest dependency is a polyfill used for the block editor. A new version is available so this should be updated.

What's not immediately clear is what this polyfill is used for and if it's still required. It's in the Gutenberg documentation, but is not defined as a dependency within that repository (most likely because it has been in Core for some time).

The update has relatively minor changes, but confirming that the areas of Core relying on this polyfill continue to work properly would be a good step to take.

Currently, version 2.0.2 is bundled with Core. The latest is now v3.0.2.

A full list of changes can be found on GitHub: https://github.com/jonathantneal/closest/blob/master/CHANGELOG.md#302-october-31-2019

Attachments (2)

52851.diff (1.6 KB) - added by desrosj 3 years ago.
52851.2.diff (1.6 KB) - added by desrosj 3 years ago.

Download all attachments as: .zip

Change History (21)

@desrosj
3 years ago

#1 @desrosj
3 years ago

  • Keywords has-patch needs-testing added
  • Type changed from defect (bug) to enhancement

This ticket was mentioned in Slack in #core-editor by desrosj. View the logs.


3 years ago

#3 @Hareesh Pillai
3 years ago

Patch looks good and the QUnit tests have all passed. Based on my testing of a few areas that use .closest, it seems to work fine.

I suggest that we commit this patch so that we could catch any edge cases.

@desrosj
3 years ago

#4 @desrosj
3 years ago

52851.2.diff is just a refresh to apply to trunk cleanly.

I was looking at this again today. It seems that the name of the file in the package has changed. Previously, the package contained an element-closest.js file that was used in the Webpack build process.

In the latest release, it seems that browser.js is the result of a new build process and is meant to replace this file. However, the file is also minified. Core currently has an expanded and minified version of wp-polyfill-element-closest.js, so this will need to be addressed in the build process.

#5 @gziolo
3 years ago

As far as I can tell we should be able to use one of the unminified files:

So we should need to update references in both vendors and minifiedVendors.

#6 @desrosj
3 years ago

  • Keywords needs-refresh added
  • Milestone changed from 5.8 to Future Release

This is not mission critical, and I have not had a chance to revisit this.

Going to punt.

#7 @desrosj
20 months ago

  • Milestone changed from Future Release to 6.2

Would be nice to wrap this up in 6.2. The library has not been updated since 2019, so 3.x is likely the last version.

This ticket was mentioned in PR #3407 on WordPress/wordpress-develop by desrosj.


20 months ago
#8

  • Keywords needs-refresh removed

@mukesh27 commented on PR #3407:


18 months ago
#9

@desrosj Some how the map file is not copied from node_modules folder.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


16 months ago

#11 @mukesh27
16 months ago

  • Keywords changes-requested added

#12 @mukesh27
16 months ago

This ticket was discussed in the recent bug scrub.

Thanks @desrosj for the ticket and PR.

The tests on PR going fails do you have moment to take a look? We have beta in few week.

Props to @costdev

This ticket was mentioned in Slack in #core by costdev. View the logs.


16 months ago

@desrosj commented on PR #3407:


16 months ago
#14

@desrosj Some how the map file is not copied from node_modules folder.

To be clear, the map file not being copied over is the correct behavior. This error is happening because there is now a //# sourceMappingURL=index.js.map line at the bottom of the unminified index.js file for the package.

#15 @desrosj
16 months ago

  • Keywords changes-requested removed

I've gone and refreshed the PR against the current state of trunk.

Thinking about this more, I'm leaning towards just using the minified version of the package in both versions of the file created.

It's not used in Core anymore and is only maintained as a courtesy and to prevent 404 errors if being loaded directly. Additionally, there's almost no real world usage of this polyfill according to a search of the plugin library.

I've also included this change in the PR. If this seems like the right route, I'll get it committed for 6.2. Otherwise we should just maybelater this one as there's really very little benefit.

#16 @desrosj
16 months ago

  • Owner set to desrosj
  • Status changed from new to assigned

#17 @desrosj
16 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55275:

External Libraries: Update element-closest.

This updates the element-closest polyfill to the latest version, 3.0.2.

Because of changes to how the package is now built and distributed, both files for the library now contain minified code.

This library is no longer used by Core itself and maintained as a courtesy. Any projects utilizing it should reevaluate their usage requirements with modern browsers.

Props hareesh-pillai, gziolo, mukesh27, costdev.
Fixes #52851.

#19 @milana_cap
15 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.