Update `@vercel/ncc` to v0.29.2 #6605

TooTallNate · 2021-08-17T08:24:50Z

No description provided.

codecov · 2021-08-25T01:50:06Z

Codecov Report

Merging #6605 (ab77706) into main (aca42b2) will increase coverage by 0.85%.
The diff coverage is n/a.

❗ Current head ab77706 differs from pull request most recent head 9877edf. Consider uploading reports for the commit 9877edf to get more accurate results

@@            Coverage Diff             @@
##             main    #6605      +/-   ##
==========================================
+ Coverage   12.53%   13.39%   +0.85%     
==========================================
  Files         265      265              
  Lines        9637     9758     +121     
  Branches     1726     1664      -62     
==========================================
+ Hits         1208     1307      +99     
- Misses       8330     8362      +32     
+ Partials       99       89      -10

Impacted Files	Coverage Δ
src/util/dev/builder-cache.ts	`67.87% <0.00%> (+3.40%)`	⬆️
src/util/dev/server.ts	`52.89% <0.00%> (+3.95%)`	⬆️

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 aca42b2...9877edf. Read the comment docs.

TooTallNate · 2021-08-25T21:45:33Z

packages/cli/scripts/build.ts

+  const args = [
+    'ncc',
+    'build',
+    '--no-asset-builds',


Needed to enable this new flag to prevent builder-worker.js and vercel/fun runtimes-related .js files from being compiled by ncc.

Will post a PR to track possibly disabling this feature by default.

TooTallNate · 2021-08-25T21:46:09Z

packages/cli/scripts/build.ts

-  // A bunch of source `.ts` files from CLI's `util` directory
-  await remove(join(dirRoot, 'dist', 'util'));
+  // Band-aid to bundle stuff that `ncc` neglects to bundle
+  await cpy(join(dirRoot, 'src/util/projects/VERCEL_DIR_README.txt'), distRoot);


For some reason, the VERCEL_DIR_README.txt file is not being included in the ncc output dir 🤔

Oh I see, it is passed to readFile here:

vercel/packages/cli/src/util/projects/link.ts

Lines 234 to 237 in ee9a8a0

await writeFile(

join(path, VERCEL_DIR, VERCEL_DIR_README),

await readFile(join(__dirname, 'VERCEL_DIR_README.txt'), 'utf8')

);

So that does seem like a bug.

Thoughts @guybedford ?

In package/cli/src/util/projects/link.ts you're using __dirname in an ES module and __dirname isn't defined in ES modules.

I think you should rather do something like await readFile(new URL('VERCEL_DIR_README.txt', import.meta.url)).

This package cli is using target: commonjs so I don't think we should use ESM import. I know that __dirname would work with tsc so I think it should work with ncc too.

vercel/packages/cli/tsconfig.json

Line 7 in aca42b2

"module": "commonjs",

Ok that seemed to work (d617e80). However I had to add a global definition for the url property otherwise I was hitting error:

error: TS2339 [ERROR]: Property 'url' does not exist on type 'ImportMeta'.

Yeah unfortunately TypeScript are dragging their feet on ESM conventions as usual.

I reverted it actually cause it was causing an error in the unit tests. Will look into it again in a separate PR.

TooTallNate · 2021-08-25T21:46:39Z

packages/cli/src/util/dev/builder-cache.ts

@@ -19,7 +19,7 @@ import { BuilderWithPackage } from './types';

 type CliPackageJson = typeof cliPkg;

-declare const __non_webpack_require__: typeof require;
+const require_: typeof require = eval('require');


Is this the most "sanctioned" way (as far as ncc is concerned) to get access to the real require() function?

eval('require') is literally what I would recommend yes. In general eval() as the way around ncc has sort of become a pattern as anything else is game for ncc. For this reason specifically we don't actually eval eval although we could...

TooTallNate · 2021-08-25T21:48:20Z

packages/cli/src/util/dev/builder-cache.ts

@@ -355,8 +355,8 @@ export async function getBuilder(

    try {
      output.debug(`Requiring runtime: "${requirePath}"`);
-      const mod = require(requirePath);
-      const pkg = require(join(requirePath, 'package.json'));
+      const mod = require_(requirePath);


Previous versions of ncc would allow dynamic require calls to still work, but as of this version of ncc it throws a webpack-specific "Module not found" error, so using the native require() function was necessary.

Unknown requires should delegate to the environment require. But Webpack is a moving target too in ensuring the invariants here. I will follow up with an ncc issue.

Was trying to replicate this and couldn't then realised this is the same issue - this is a TS ESM file so "require" is not defined.

Rather use dynamic import or createRequire to do a dynamic require / import from an ES module.

TooTallNate · 2021-08-25T21:50:09Z

packages/node/test/fixtures/21-esm-lit/package.json

@@ -1,7 +1,7 @@
 {
  "type": "module",
  "dependencies": {
-    "@lit-labs/ssr": "1.0.0-rc.1",
-    "lit": "2.0.0-rc.2"
+    "@lit-labs/ssr": "1.0.0-rc.2",


Unrelated to the ncc update itself, but this fixture's deps had a transient dependency updated that was published broken to npm. Updated the deps here and added a lockfile to prevent that from happening in the future.

TooTallNate · 2021-08-25T21:50:32Z

packages/node/tsconfig.json

@@ -8,6 +8,7 @@
    "outDir": "dist",
    "sourceMap": false,
    "declaration": true,
+    "moduleResolution": "node",


Latest version of ncc (correctly) complained about this missing prop. All other packages have it already.

styfle · 2021-08-25T22:07:51Z

packages/node-bridge/launcher.js

@@ -86,7 +86,8 @@ function getVercelLauncher({
          if (shouldAddHelpers) {
            bridge.setStoreEvents(true);
            import(helpersPath).then(helper => {
-              const server = helper.createServerWithHelpers(listener, bridge);
+              const h = helper.default || helper;
+              const server = h.createServerWithHelpers(listener, bridge);


I'm curious, was this changed because the ncc output of helpers changed? Maybe we shouldn't use default export for the helpers?

I didn't really understand this change either. Could be related to Node.js version, because I didn't need this change locally, but on CI it was failing and I needed to check for default in that case.

~~Not using default export is an interesting idea though.~~ We're not actually using default export or module.exports in this case…

The only reason I can think this would happen is if helpers is being built into CommonJS as a separate file, and is not part of the ncc build here. Hence for Node.js versions without named exports support for CommonJS it could fail.

If it is referencing the original TS build file as a reference outside of the build, the fix would be to build into ESM not CJS.

And if the above assumption is correct, then the helper.default should work in all cases and the || helper fallback shouldn't be necessary.

Would still be curious how this one works out with const h = helper.default;.

styfle · 2021-08-25T22:09:43Z

packages/cli/src/util/dev/builder-cache.ts

-  const _require =
-    typeof __non_webpack_require__ === 'function'
-      ? __non_webpack_require__
-      : require;


I would expect this to still work since ncc is using webpack under the hood.

Maybe @guybedford could explain why it doesn't work?

Perhaps it should be __nccwpck_require__ instead?

This part still works, but is unnecessary because of the eval('require') version.

This reverts commit d617e80.

Update @vercel/ncc to v0.29.1

9af927e

vercel bot deployed to Preview August 17, 2021 08:24 View deployment

Add --no-asset-builds flag to CLI ncc build

111003b

vercel bot deployed to Preview August 18, 2021 19:38 View deployment

Update to @vercel/ncc v0.29.2

8b580b9

vercel bot deployed to Preview August 19, 2021 17:45 View deployment

TooTallNate changed the title ~~Update @vercel/ncc to v0.29.1~~ Update @vercel/ncc to v0.29.2 Aug 21, 2021

Merge branch 'main' of github.com:vercel/vercel into update/ncc-0.19.1

194ecb9

vercel bot deployed to Preview August 24, 2021 18:11 View deployment

Typescript external

8a3e8bd

vercel bot deployed to Preview August 24, 2021 18:22 View deployment

Test fix @vercel/node

8e9adc9

vercel bot deployed to Preview August 24, 2021 19:26 View deployment

Copy VERCEL_DIR_README.txt to the "dist" dir

e00dd58

vercel bot deployed to Preview August 24, 2021 19:59 View deployment

require() hack :/

907672e

vercel bot deployed to Preview August 24, 2021 20:10 View deployment

Attempt fix

04253bd

vercel bot deployed to Preview August 24, 2021 21:09 View deployment

Use Node require() in @vercel/node

c2082f3

vercel bot deployed to Preview August 25, 2021 01:43 View deployment

Debug

4c0dd14

vercel bot deployed to Preview August 25, 2021 05:00 View deployment

Attempt fix

bac01ae

vercel bot deployed to Preview August 25, 2021 05:14 View deployment

Update 21-esm-lit test fixture dependencies

b93db6b

vercel bot deployed to Preview August 25, 2021 20:04 View deployment

Remove debug logging

852f26d

vercel bot deployed to Preview August 25, 2021 20:25 View deployment

TooTallNate marked this pull request as ready for review August 25, 2021 21:18

TooTallNate requested review from AndyBitz, coetry, lucleray and styfle as code owners August 25, 2021 21:18

TooTallNate commented Aug 25, 2021

View reviewed changes

styfle reviewed Aug 25, 2021

View reviewed changes

Merge branch 'main' of github.com:vercel/vercel into update/ncc-0.19.1

4529e24

vercel bot deployed to Preview August 26, 2021 01:30 View deployment

Use import.meta.url

d617e80

vercel bot deployed to Preview August 26, 2021 22:57 View deployment

Remove unnecessary

05e4cd8

vercel bot deployed to Preview August 26, 2021 23:00 View deployment

Revert "Use import.meta.url"

9877edf

This reverts commit d617e80.

vercel bot deployed to Preview August 27, 2021 00:30 View deployment

styfle approved these changes Aug 27, 2021

View reviewed changes

TooTallNate merged commit f221f04 into main Aug 27, 2021

TooTallNate deleted the update/ncc-0.19.1 branch August 27, 2021 17:03

styfle mentioned this pull request Sep 21, 2021

[all] Revert to @vercel/ncc@0.24.0 #6749

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `@vercel/ncc` to v0.29.2 #6605

Update `@vercel/ncc` to v0.29.2 #6605

TooTallNate commented Aug 17, 2021

codecov bot commented Aug 25, 2021 •

edited

TooTallNate Aug 25, 2021

guybedford Aug 26, 2021

TooTallNate Aug 25, 2021

styfle Aug 25, 2021 •

edited

guybedford Aug 26, 2021

styfle Aug 26, 2021

TooTallNate Aug 26, 2021 •

edited

guybedford Aug 26, 2021

TooTallNate Aug 27, 2021

TooTallNate Aug 25, 2021

guybedford Aug 26, 2021

TooTallNate Aug 25, 2021

guybedford Aug 26, 2021

guybedford Aug 26, 2021 •

edited

TooTallNate Aug 25, 2021

TooTallNate Aug 25, 2021

styfle Aug 25, 2021

TooTallNate Aug 25, 2021 •

edited

guybedford Aug 26, 2021

guybedford Aug 26, 2021

guybedford Aug 26, 2021

styfle Aug 25, 2021 •

edited

TooTallNate Aug 25, 2021

	await writeFile(
	join(path, VERCEL_DIR, VERCEL_DIR_README),
	await readFile(join(__dirname, 'VERCEL_DIR_README.txt'), 'utf8')
	);

Update @vercel/ncc to v0.29.2 #6605

Update @vercel/ncc to v0.29.2 #6605

Conversation

TooTallNate commented Aug 17, 2021

codecov bot commented Aug 25, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

styfle Aug 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TooTallNate Aug 26, 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

guybedford Aug 26, 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

TooTallNate Aug 25, 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

styfle Aug 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Update `@vercel/ncc` to v0.29.2 #6605

Update `@vercel/ncc` to v0.29.2 #6605

codecov bot commented Aug 25, 2021 •

edited

styfle Aug 25, 2021 •

edited

TooTallNate Aug 26, 2021 •

edited

guybedford Aug 26, 2021 •

edited

TooTallNate Aug 25, 2021 •

edited

styfle Aug 25, 2021 •

edited