A polyfill for String.includes() is needed for Internet Explorer 11.
This is one of the three polyfills from #3076171: Provide a new library to replace jQuery UI autocomplete and we are now addressing each in their own issue.
Note: While this polyfill is currently a nice-to-have, it will be necessary in order for the new Autocomplete issue #3076171: Provide a new library to replace jQuery UI autocomplete to land as it uses a JS library that makes use of String.includes. That issue's MR is very large, so adding the String.includes polyfill in this separate issue helps make that one more manageable.
In general, more JS libraries are being built that assume String.includes is available, so I believe this is a good polyfill to have around for reasons beyond #3076171: Provide a new library to replace jQuery UI autocomplete. For example, a need for it has come up in #3076171: Provide a new library to replace jQuery UI autocomplete, too. For review-ability purposes, it is preferable to get this added in a targeted issue vs adding it as part of a larger scope.
A polyfill for String.includes has been added to Drupal core.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | tour_uninstalled_includes_check.mp4 | 3.38 MB | hooroomoo |
| #5 | includes_polyfill_usage.mp4 | 3.3 MB | hooroomoo |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
bnjmnmIn an issue where a polyfill is added, there's a step that should be included when possible: if there are instances where existing code has workarounds due to this polyfill not existing, at least one instance of that workaround should be changed to use the now-polyfilled function, and there should be evidence that the non-workaround version works... particularly in IE11 (or whatever other browser needs to be polyfilled, but TBH it's pretty much always IE11)
Currently, Drupal core has plenty of places where it would work best to use
string.includes, but the lack of IE11 support means it's necessary to use the approach in the top voted answer in this Stackoverflow thread.. Find one of these uses ofindexOf !== -1and replace it with the more elegantstring.includes()... and add thestring.includeslibrary as a dependency of the library serving the file you just changed. For example, something likeoptions.firstCharacterBlacklist.indexOf(term[0]) !== -1in autcomplete.es6.js could instead useoptions.firstCharacterBlacklist.includes(term[0])Evidence that the
string.includesapproach works is easier if it is something with test coverage. However, our tests don't cover IE11, so you'll need to manually test it there and provide video/screenshot evidence that IE11 is correctly handling the use ofstring.includesWhen deciding which use of
indexOfto refactor tostring.includes, I suggest finding something that includes code that you can easily invoke in a local instance. That makes it easy to create the proof that the polyfill is doing its job.Changing it in 1-2 places is sufficient for this issue, but a followup issue should be created to refactor all uses of the
indexOfworkaround withstring.includesonce the polyfill is part of core.Comment #4
hooroomooComment #5
hooroomooScreen recording of polyfill usage (#4) in IE11 for admn block filtering functionality.
Things to note that may or may not be an issue:
1. Polyfill only worked when "Aggregate Javascript files" was unchecked from Bandwidth Optimization in the setting of admin/config/development/performance
2. Getting an AJAX error at timestamp 0:06 in the video.
Comment #6
bnjmnmre #5
Looks like that was due to aggregator being aware
block.admin.jswas updated (introducing the use ofinclude()), but not aware the polyfill files were added, so it failed as a result. There was another unrelated error in that video, which turned out to be a yet-to-be-reported bug. I've filed that #3240145: Shepherd JS not IE11 compatible.Another version of the video in #5, but with the cache rebuilt + the Tour module disabled should be sufficient evidence that the polyfill works properly in IE11.
Comment #7
bnjmnmTagging with "needs change record". A change record can be added by clicking the "add change notice" link in the right sidebar. The content of the CR can largely be pasted in from this existing CR from a similar issue #3176383: JavaScript NodeList.forEach polyfill library added, then making the handful of changes needed to make it specific to this one.
Comment #8
hooroomooVideo of string.includes usage on IE 11 with tour uninstalled and cache rebuilt
Comment #9
hooroomooComment #10
larowlanLooks good to me, change record is done, removing that tag
Comment #11
lauriiiPosted question on the MR
Comment #12
bnjmnmThe CR and polyfill look good. One usage of the indexOf workaround has been changed to use
includes()in block.admin.es6.js. This is functionality that has coverage in\Drupal\Tests\block\FunctionalJavascript\BlockFilterTest::testBlockFilterso that is confirmation the change toincludes()didn't break any JS, and #8 demonstrates that the polyfill allows IE11 to use code that invokes String.includes.Comment #13
larowlanIssue credits
Comment #16
larowlanI merged this and then reverted it because I didn't realise we were in alpha freeze.
I'm not sure what that means in terms of needing the MR to be reopened?
Comment #18
bnjmnmI created a new MR:
3239509-post-freezeto potentially make it easier to re-commit these changes. It's a new commit hash to avoid any history collisions (not sure if that's a concern with the Gitlab workflow, but I took the precaution).Comment #19
larowlanRebased this on 9.4.x
Checked with catch who said there seemed like little risk of disruption in backporting this.
Waiting for a green run
Comment #22
larowlanCommitted to 9.4.x, backported to 9.3.x
Republished the change notice.
Thanks folks