upgrade to webpack v5 stable version #602

huozhi · 2020-10-13T05:44:16Z

since webpack 5 stable version is released, try to adopt it to ncc

styfle

Looks like the watcher API changed causing the test to fail.

Previously, it was changed from expect(missing.length).toBeGreaterThan(100) to expect(missing._set.size).toBeGreaterThan(100) in PR #579 so perhaps you could try changing it back?

huozhi · 2020-10-13T13:34:28Z

looks like the previous code doesn't work as well. I logged the missing._set, missing._set is always 43 on my side. do you know why it's expecte to be over 100 and what does this condition mean? @styfle @guybedford

huozhi · 2020-10-13T13:37:40Z

webpack 5.0.0-beta.29 includes a fixing "fix watching when more than 2000 directories are watched on MacOS", could this be related to it? sounds like webpack tried to reduce the watchers count, so less than 100 might be expected?

styfle · 2020-10-13T14:09:50Z

But the failures are only happening on Linux, macOS passes CI

huozhi · 2020-10-23T15:13:54Z

Note added:

sometimes the CI failed with test "Should support custom watch API" that missing._set.size is less than 100 on ubuntu VM. I also encounter this issue sometimes on my macbook laptop with the exact same error message. after I logged the missing._set I found that the file paths inside are not correct. like '/Users/myname/code/ncc/Users/myname/package.json' the Users/myname has been duplicated 2 times.

styfle · 2020-10-23T15:16:09Z

I found that the file paths inside are not correct

Sounds like a webpack bug?

huozhi · 2020-10-24T04:37:52Z

Founding

I'm doubting it's a issue while webpack working with twilio.js since twilio requires native nodejs module and webpack 5 won't polyfill them. I created a gist to run webpack@5 with custom watch (same with watcher.test.js) to compile twilio.js (same with test/integration/twilio.js) 👉 link

and I got logs below

Module not found: Error: Can't resolve 'util' in '/Users/huozhi/Code/test/webpack5/node_modules/twilio/lib/twiml'                                                                                  [45/4307]

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "util": require.resolve("util/") }'
        - install 'util'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "util": false }
 @ ./node_modules/twilio/lib/index.js 48:46-78
 @ ./twilio.js 1:15-32

ERROR in ./node_modules/twilio/lib/webhooks/webhooks.js 3:13-30
Module not found: Error: Can't resolve 'crypto' in '/Users/huozhi/Code/test/webpack5/node_modules/twilio/lib/webhooks'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "crypto": require.resolve("crypto-browserify") }'
        - install 'crypto-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "crypto": false }
 @ ./node_modules/twilio/lib/index.js 4:15-45
 @ ./twilio.js 1:15-32

currently ncc swallowed these errors that I can't see them even turn the webpack stats.logging flag to "verbose".

in webpack@5.0.0-beta.31 they add this change: add resolve.fallback option and recommend that for polyfilling. I guess that's why it breaks after bumping webpack version higher than beta-28.

Followup

does ncc want to support nodejs fallback out of box?
or user need to provide fallback aliases like webpack suggested?

styfle · 2020-10-24T21:47:52Z

Nice find!

I was thinking the Node.js fallback was removed during the webpack beta but I guess we are a bit outdated as you mentioned.

does ncc want to support nodejs fallback out of box?

I think we need to have the Node.js fallback out of the box like the old webpack because ncc is designed specifically for Node.js

styfle · 2020-10-25T04:20:39Z

test/watcher.test.js

@@ -97,7 +97,7 @@ jest.setTimeout(30000);

 it('Should support custom watch API', async () => {
  let buildCnt = 0;
-  const buildFile = path.resolve('./test/integration/request.js');
+  const buildFile = path.resolve('./test/integration/twilio.js');


Does it work work with request now that node-libs-browser is added?

request.js will work for both whatever you add the nodejs lib fallback support or not. twilio.js doesn't work without fallback that breaks the watcher test case.

I changed the original file from twilio to request to let it pass the test in last commit.

src/index.js

styfle · 2020-10-25T20:04:16Z

src/index.js

@@ -205,8 +205,7 @@ module.exports = (
      mainFields: ["main"],
      plugins: resolvePlugins
    },
-    // https://github.com/vercel/ncc/pull/29#pullrequestreview-177152175
-    node: false,
+    node: {},


Is {} different than false?

I'm looking at the docs between v4 and v5 and the false value seems to be the same for both.

https://v4.webpack.js.org/configuration/node/

https://webpack.js.org/configuration/node/

I thought it was the root cause to trigger the error but looks like it just accidently passed tests..after rerun the tests they're still failed. reverted

wonder why CI gets passed now...seems the nodejs lib fallback wasn't the issue before? I'm still stucking at watcher test failure on my local laptop 🤔 ...

styfle

Looks good now, thanks!

huozhi requested review from styfle and Timer as code owners October 13, 2020 05:44

huozhi changed the title ~~upgrade to webpack 5~~ upgrade to webpack v5 stable version Oct 13, 2020

styfle reviewed Oct 13, 2020

View reviewed changes

huozhi added 4 commits October 23, 2020 23:07

upgrade to webpack 5

a73d426

remove duplicated semi

074f448

try 5.1.3

523c3fc

5.2.0

898b9c3

switch twilio.js to request.js for watcher test

9d6ac9b

huozhi requested a review from styfle October 24, 2020 04:38

add nodejs lib fallback

a355b39

styfle reviewed Oct 25, 2020

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

huozhi added 2 commits October 25, 2020 15:00

set node = true

f216e7f

set node = true

486d6c1

huozhi requested a review from styfle October 25, 2020 12:46

styfle reviewed Oct 25, 2020

View reviewed changes

huozhi added 2 commits October 26, 2020 14:50

revert

5081643

Merge branch 'master' into upgrade-webpack-5

9417fc2

huozhi requested a review from styfle October 27, 2020 00:41

styfle added automerge Automatically merge PR once checks pass hacktoberfest-accepted labels Oct 31, 2020

styfle approved these changes Oct 31, 2020

View reviewed changes

kodiakhq bot merged commit 8e1e5fc into vercel:master Oct 31, 2020

huozhi deleted the upgrade-webpack-5 branch November 1, 2020 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade to webpack v5 stable version #602

upgrade to webpack v5 stable version #602

huozhi commented Oct 13, 2020 •

edited

styfle left a comment

huozhi commented Oct 13, 2020

huozhi commented Oct 13, 2020

styfle commented Oct 13, 2020

huozhi commented Oct 23, 2020

styfle commented Oct 23, 2020

huozhi commented Oct 24, 2020 •

edited

styfle commented Oct 24, 2020 •

edited

styfle Oct 25, 2020

huozhi Oct 25, 2020

styfle Oct 25, 2020

huozhi Oct 26, 2020

huozhi Oct 26, 2020

styfle left a comment

upgrade to webpack v5 stable version #602

upgrade to webpack v5 stable version #602

Conversation

huozhi commented Oct 13, 2020 • edited

styfle left a comment

Choose a reason for hiding this comment

huozhi commented Oct 13, 2020

huozhi commented Oct 13, 2020

styfle commented Oct 13, 2020

huozhi commented Oct 23, 2020

styfle commented Oct 23, 2020

huozhi commented Oct 24, 2020 • edited

Founding

Followup

styfle commented Oct 24, 2020 • edited

styfle Oct 25, 2020

Choose a reason for hiding this comment

huozhi Oct 25, 2020

Choose a reason for hiding this comment

styfle Oct 25, 2020

Choose a reason for hiding this comment

huozhi Oct 26, 2020

Choose a reason for hiding this comment

huozhi Oct 26, 2020

Choose a reason for hiding this comment

styfle left a comment

Choose a reason for hiding this comment

huozhi commented Oct 13, 2020 •

edited

huozhi commented Oct 24, 2020 •

edited

styfle commented Oct 24, 2020 •

edited