Road to v4 🎉 #2594

Markel · 2020-08-30T13:45:34Z

This PR supersedes #2108 so it closes #2108

The objetive of the PR is to finish the work started by @patrickkettner of adapting Modernizr to es6 modules. I will try to solve all merging conflicts as they are created so the PR can be picked up by anyone easily.

Compare last Patrick's commit to this PR

TODO:
(Bold means currently working on it)

Solve merging conflicts
Solve the errors
Convert some tests to es6 (mainly because they are recent like the avif test)
Read through the PR again to move the comments here
Solve all the problems that convert to es6 modules #2108 has
Check for _globalThis in comments
Check and possibly rewrite scripts/flatten-for-publishing.js
Check different browsers with v3 vs. v4 to see if any test is broken (as a safety net)
Maybe add some documentation for the transition to v4?

Markel · 2020-08-30T13:49:09Z

Note: I know that I usually have signed commits, but I'm currently moving for the student year so I'm working from a new PC and I haven't set up the signed commits yet.

Imported from: https://gist.github.com/Markel/d587ab198a9e1552c0cf0eddcff4f313

* Recreate package.lock Some problems were created when they new commits were merged * Solve hyphens and data-uri

Tests converted: scrolltooptions, resizeobserver & clipboard

* Honestly I'm fu***** stupid More merging errors * Change tests so errors don't appear 🧠

Markel · 2020-09-01T13:03:55Z

I think I've never been so happy to see the codecov comment, because it means that the PR is finally stable :))))) (apart from appveyor's node 14)

The only thing that worries me is the amount of tests deleted from the test\node\lib\metadata.js file, but I basically followed @patrickkettner 's edits, maybe he could explain why he deleted some tests.

I will be busy the rest of the day and probably tomorrow, so I will read the previous PR in some days.

Markel · 2020-09-01T15:11:20Z

Sorry, that was an error 😅

Update to x64 version of node

Markel · 2020-09-10T14:08:29Z

Sorry for the hard reset yesterday, I didn't have time to solve the error yesterday and it failed the tests so I hard reverted the commit (actually first time I had done it). Hope it didn't affect ongoing reviews, I'll avoid forced pushes in the future. PS: I haven't merged fda7369, but you know, it merges automatically, it will get merged when a new conflicting commit appears.

codecov · 2020-10-02T18:47:47Z

Codecov Report

Merging #2594 (544793c) into master (3514072) will increase coverage by 3.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2594      +/-   ##
==========================================
+ Coverage   95.18%   98.52%   +3.34%     
==========================================
  Files           5       50      +45     
  Lines         166      544     +378     
==========================================
+ Hits          158      536     +378     
  Misses          8        8

Impacted Files	Coverage Δ
lib/metadata.js	`94.82% <0.00%> (-5.18%)`	⬇️
lib/cli.js	`100.00% <0.00%> (ø)`
lib/options.js	`100.00% <0.00%> (ø)`
gulpfile.babel.js
src/domPrefixesAll.js	`100.00% <0.00%> (ø)`
src/roundedEquals.js	`100.00% <0.00%> (ø)`
src/getBody.js	`100.00% <0.00%> (ø)`
src/omPrefixes.js	`100.00% <0.00%> (ø)`
src/injectElementWithStyles.js	`100.00% <0.00%> (ø)`
src/domToCSS.js	`100.00% <0.00%> (ø)`
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1fbec3...c8f62e1. Read the comment docs.

Markel · 2020-10-02T19:43:15Z

I was thinking that we may use the PR to also bump some breaking updates to some dependencies, the best of all? Most of them don't even break inside Modernizr (the irony)

All packages except the mocha ones can be updated to the latest version without requiring any changes (or at least npm test doesn't break. I will investigate about the mocha errors and comment any discoveries that I find. So does anyone have any problem with updating the dependencies?

patrickkettner · 2020-10-02T21:12:41Z

only reason older mocha was being used was so that the test suite could run in older browsers, to confirm that detects were actually detecting things

…

On Fri, Oct 2, 2020 at 3:43 PM Markel F. ***@***.***> wrote: I was thinking that we may use the PR to also bump some breaking updates to some dependencies, the best of all? Most of them don't even break inside Modernizr (the irony) [image: Outdated packages list] <https://camo.githubusercontent.com/8e4a5a8835dc99802881069650ceec5f39536db3/68747470733a2f2f692e696d6775722e636f6d2f6d4b774c7330682e706e67> All packages except the mocha ones can be updated to the latest version without requiring any changes (or at least npm test doesn't break. I will investigate about the mocha errors and comment any discoveries that I find. So does anyone have any problem with updating the dependencies? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2594 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUBRZG4LQ6DBZXDRCDO3SIYUODANCNFSM4QPUEYXA> .

-- patrick

Markel · 2020-10-03T14:11:07Z

So due to the es6 imports being used now, those browsers won't work (at least not in a build) won't they? So would that mean that we can upgrade mocha? Correct me if I'm wrong

patrickkettner · 2020-10-03T15:08:14Z

Id hope their compiled output would work without modules? Otherwise modernizr looses a lot of its utility (based on its usage), since evergreen browser features are so much easier to test

On Sat, Oct 3, 2020 at 10:11 AM Markel F. ***@***.***> wrote: So due to the es6 imports being used now, those browsers won't work (at least not in a build) won't they? So would that mean that we can upgrade mocha? Correct me if I'm wrong — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2594 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUBQMJHSZJWSCE37DJ2LSI4WIRANCNFSM4QPUEYXA> . --

patrick

Markel · 2020-10-03T15:45:40Z

Yeah, that was my idea when I wrote the correct me if I'm wrong, I mean it wouldn't make to much sense to have tests for es5 if you require es6 to run the test. But I haven't touched too much the PR except for solving all the merge conflicts, so I'm still checking some technical details about it. But yeah, I think that rollup will help on not requiring es6. Thanks!

Keyword 'for' in `Symbol.for` caused `Undefined identifier` error in IE8.

I had started solving the conflicts before that commit was merged

Markel · 2021-01-18T17:30:19Z

Note: I'm moving all the comments from #2108 to here, I'll react with a 👍 if I solved it (some easy things) and 👀 if I copied the comment (reactions in the original PR). Also I'm taking ownership from all your comments @rejas as I have to rewrite them 😈

Markel · 2021-01-18T17:30:33Z

bin/modernizr

@@ -133,16 +124,16 @@ if (argv.o || argv.f) {
        // special case around it :[
        if (prop === 'setClasses') {
          return 'setClasses';
-        } else if (_.isUndefined(obj)) {
-          throw new Error('invalid key value name - ' + prop);
+        } else if (typeof obj === 'undefined') {


isnt (obj === undefined ) enough these days?

Markel · 2021-01-18T17:31:03Z

bin/modernizr

@@ -39,30 +37,23 @@ var yargs = require('yargs')
    alias: 'uglify',
    describe: 'uglify/minify the output'
  })
-  .options('s', {


this option is gone for good?

Markel · 2021-01-18T17:33:32Z

feature-detects/css/gradients.js


-    if (Modernizr._config.usePrefixes) {
-    // legacy webkit syntax (TODO:: remove when syntax not in use anymore)


so the TODO isnt valid anymore? it will never be removed?

Markel · 2021-01-18T17:41:14Z

feature-detects/elem/ruby.js

+import Modernizr from '../../src/Modernizr.js';
+import createElement from '../../src/createElement.js';
+import docElement from '../../src/docElement.js';
+import computedStyle from '../../src/computedStyle.js';


Good catch, but would be good to have changes in the feature detects separately...

(Not really sure what you mean by this...)

Markel · 2021-01-18T17:44:04Z

feature-detects/elem/track.js

+Modernizr.addTest('texttrackapi', results.texttrackapi);
+
+// a more strict test for track including UI support: document.createElement('track').kind === 'subtitles'
+Modernizr.addTest('track', 'kind' in createElement('track'));


shouldnt this just be results.track?

Markel · 2021-01-18T17:50:10Z

feature-detects/postmessage.js

@@ -16,15 +16,20 @@
 }
 !*/
 /* DOC
-Detects support for the `window.postMessage` protocol for cross-document messaging.
+Detects support for the `self.postMessage` protocol for cross-document messaging.


Yeah it doesn't seem to make too much sense

Markel · 2021-01-18T17:55:04Z

feature-detects/textencoding.js

+export let textencoder = Modernizr.textencoder
+export let textdecoder = Modernizr.textdecoder


why is here an export let? why is that special?

Markel · 2021-01-18T18:07:25Z

lib/config-all.json

-    "load",
    "mq",
    "prefixed",
    "prefixes",
    "prefixedCSS",
-    "setClasses",


why is that gone?

and could you document those changes (in the options) in the README too?

Markel · 2021-01-18T18:08:30Z

lib/options.js

+      .reduce((acc, val) => acc.concat(val), [])
+      .reduce((acc, val) => acc.concat(val), [])


twice the same call to reduce?

Markel · 2021-01-18T18:13:47Z

src/html5shiv.js

@@ -1,338 +1,339 @@
-define(['isSVG'], function(isSVG) {


that would be a thing: an update to html5shiv for es6 :-)

Markel · 2021-01-18T18:15:32Z

src/isBrowser.js

+var isBrowser = typeof self !== 'undefined' && typeof self.document !== 'undefined';
+export default isBrowser


some documentation when this should be used would be needed.

Markel · 2021-01-18T18:23:55Z

test/browser/src/testProps.js

      // Everyone supports fontSize
-      expect(testProps(['fontSize'])).to.be.equal(true);
+      expect(testProps(['fontSize'])).to.equal(true);
      // kebab-case should work too


you dont like the "be"s? 🐝

Markel · 2021-01-19T17:59:57Z

Just looking for other things a found this little irony 😅 #1786 (comment)

Have a good day everyone and stay safe :)

niksy · 2021-10-01T05:18:43Z

Is this still active? What can we do to help move this along?

Markel · 2021-10-02T20:33:11Z

Is this still active? What can we do to help move this along?

Hi @niksy. Honesty, I had quite forgotten of the PR, there are merge conflicts should be easy to solve (as not too many commits have been done since). But honestly I don't really know what you could do to help apart from the top comments missing checkboxes 😅.

patrickkettner and others added 8 commits April 27, 2020 14:16

convert to esm

a88854c

fixes for node v10

1e65228

ESM update - fixes for windows

20f4da8

update readme, remove deprecations

8a32164

fix bad metadata

5611127

remove dupliate test

6719c54

make sure everything is using contains

a8d0baa

Merge branch 'pr/1' into amd-conversion

0729166

Markel added 4 commits August 30, 2020 15:56

Convert avif to es6

e2c4273

Imported from: https://gist.github.com/Markel/d587ab198a9e1552c0cf0eddcff4f313

Solve problems caused by merging

24f3dcf

* Recreate package.lock Some problems were created when they new commits were merged * Solve hyphens and data-uri

Convert new tests to es6

4c263c2

Tests converted: scrolltooptions, resizeobserver & clipboard

Solve errors to get a successful build! 🎊🎉

190f03f

* Honestly I'm fu***** stupid More merging errors * Change tests so errors don't appear 🧠

Solve small details

fe5dc41

Markel closed this Sep 1, 2020

Markel reopened this Sep 1, 2020

Markel and others added 3 commits September 4, 2020 14:49

Fix appveyor build (Modernizr#2597)

84b7e16

Update to x64 version of node

Merge branch 'master' into amd-conversion

346f0c7

Merge remote-tracking branch 'original/master' into amd-conversion

9b94c32

Markel force-pushed the amd-conversion branch from e956b8e to 9b94c32 Compare September 9, 2020 20:06

Update to master, sorry for the hard reset yesterday

f385ace

Solving more merge conflicts :)

6efc692

Markel mentioned this pull request Dec 20, 2020

Can we re-run tests ? #1911

Open

toddwong and others added 3 commits January 16, 2021 08:32

Fix error in es6symbol with IE8 (Modernizr#2630)

acc27ec

Keyword 'for' in `Symbol.for` caused `Undefined identifier` error in IE8.

Solve merging conflicts (except b1131f8)

1fb67e3

Solving the merge conflicts from b1131f8

1d879a2

I had started solving the conflicts before that commit was merged

Markel commented Jan 18, 2021

View reviewed changes

Perform the small changes from comments

c8f62e1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Road to v4 🎉 #2594

Road to v4 🎉 #2594

Markel commented Aug 30, 2020 •

edited

Markel commented Aug 30, 2020

Markel commented Sep 1, 2020

Markel commented Sep 1, 2020

Markel commented Sep 10, 2020 •

edited

codecov bot commented Oct 2, 2020 •

edited

Markel commented Oct 2, 2020

patrickkettner commented Oct 2, 2020 via email

Markel commented Oct 3, 2020

patrickkettner commented Oct 3, 2020 via email

Markel commented Oct 3, 2020

Markel commented Jan 18, 2021 •

edited

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel Jan 18, 2021

Markel commented Jan 19, 2021

niksy commented Oct 1, 2021

Markel commented Oct 2, 2021


		if (Modernizr._config.usePrefixes) {
		// legacy webkit syntax (TODO:: remove when syntax not in use anymore)

		export let textencoder = Modernizr.textencoder
		export let textdecoder = Modernizr.textdecoder

		.reduce((acc, val) => acc.concat(val), [])
		.reduce((acc, val) => acc.concat(val), [])

		var isBrowser = typeof self !== 'undefined' && typeof self.document !== 'undefined';
		export default isBrowser

Road to v4 🎉 #2594

Are you sure you want to change the base?

Road to v4 🎉 #2594

Conversation

Markel commented Aug 30, 2020 • edited

Markel commented Aug 30, 2020

Markel commented Sep 1, 2020

Markel commented Sep 1, 2020

Markel commented Sep 10, 2020 • edited

codecov bot commented Oct 2, 2020 • edited

Codecov Report

Markel commented Oct 2, 2020

patrickkettner commented Oct 2, 2020 via email

Markel commented Oct 3, 2020

patrickkettner commented Oct 3, 2020 via email

Markel commented Oct 3, 2020

Markel commented Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Markel commented Jan 19, 2021

niksy commented Oct 1, 2021

Markel commented Oct 2, 2021

Markel commented Aug 30, 2020 •

edited

Markel commented Sep 10, 2020 •

edited

codecov bot commented Oct 2, 2020 •

edited

Markel commented Jan 18, 2021 •

edited