Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing tests with ncc 0.20.1 #434

Closed
2 tasks done
styfle opened this issue Jun 19, 2019 · 15 comments
Closed
2 tasks done

Failing tests with ncc 0.20.1 #434

styfle opened this issue Jun 19, 2019 · 15 comments
Labels
bug Something isn't working priority Important issue or pull request to fast-track

Comments

@styfle
Copy link
Member

styfle commented Jun 19, 2019

The now-builders repo has 4 failing test fixtures and most of them appear to be regressions in ncc.

Update: Tracking issues -

Steps to reproduce

Please clone the repo and install dependencies:

git clone https://github.com/zeit/now-builders
cd now-builders
yarn
yarn bootstrap
yarn build

I went through each fixture and some fail differently with the CLI vs API.

  • CLI: this means I ran the commands you see below
  • API: this means programatic usage from @now/node index.ts

08-assets

cd packages/now-node/test/fixtures/08-assets
echo 'require("http").createServer(module.exports).listen(3000)' >> index.js 
npx @zeit/[email protected] build index.js --source-map
node dist
  • Works from CLI
  • Fails from API. Probably incorrect assets paths.

09-include-files

cd packages/now-node/test/fixtures/09-include-files
npm install
echo 'require("http").createServer(module.exports).listen(3000)' >> root.js 
npx @zeit/[email protected] build root.js --source-map
node dist
  • Fails from CLI with "RuntimeException: E_MISSING_VIEW: Cannot render root.edge. Make sure the file exists at"
  • Fails from API. This could probably be resolved by glob instead of emitting assets so hold off on changing this one for now.

14-stack-trace-ts

cd packages/now-node/test/fixtures/14-stack-trace-ts
echo 'require("http").createServer(handler).listen(3000)' >> index.ts 
npx @zeit/[email protected] build index.ts --source-map
node dist
  • Works from CLI
  • Fails from API with "callback was already called" 🤔

15-helpers

cd packages/now-node/test/fixtures/15-helpers/ts
echo 'require("http").createServer(listener).listen(3000)' >> index.ts 
npx @zeit/[email protected] build index.ts --source-map
node dist
  • Fails from CLI with "TypeError: res.status is not a function"
  • Fails from API with "callback was already called" 🤔

I think the TypeError can be ignored and probably fixing test 14 will fix 15.

@styfle styfle added bug Something isn't working priority Important issue or pull request to fast-track labels Jun 19, 2019
@guybedford
Copy link
Contributor

08-assets

The debugging information on this test shows:

Error: Fetched page https://test-g1rtmdzcg.now.sh/ does not contain asset1:190dd767190dd767190dd7,asset2:190dd767190dd767190dd7. Instead it contains An error occurred with this application.

How can we get the stack trace for whatever is happening above? I wasn't able to work out how to see this at all from the test case, so could not isolate this further.

09-include-files

This could probably be resolved by glob instead of emitting assets so hold off on changing this one for now.

I'm not sure I follow this?

I looked into the issue further and it really does come down to: do we want any __dirname reference on its own to possibly emit an entire project directory?.

The reason this becomes a problem is that we have other analysis expressions which can also resolve to dirname, eg path.resolve('.'), process.cwd() - would we want any instances of those expressions to also result in entire project directories being emitted? This is what happens in the ncc build itself in a stray process.cwd() expression resulting in the entire ncc folder being emitted.

I would strongly advise changing the test and not implementing this behaviour, but if it is absolutely necessary we can discuss very specific filtering techniques further. We need to carefully design how we want to define this space though.

14-stack-trace

Again, I have no idea how to isolate the test. If you can provide me a replication that I can hack on then I can look into this further. But since I don't know how to debug the test, I cannot determine the issue.

15-helpers

Agreed this sounds like a similar case.

@guybedford
Copy link
Contributor

I managed to trace 14 and 15 down to #435.

@guybedford
Copy link
Contributor

For 08 - if I navigate to the application directly, I can see the server outputting exactly asset1:3cbc3a523cbc3a523cbc3a,asset2:3cbc3a523cbc3a523cbc3a which is correct. Perhaps this is a test timing bug or something like that as opposed to an ncc bug.

@guybedford
Copy link
Contributor

We finally traced down 08 to the following filtering issue by building with debugLog: true:

Skipping asset emission of /tmp/74b55224/subdirectory1/asset.txt for /tmp/74b55224/index.js as it is outside the process directory /var/task

Created #440.

@guybedford
Copy link
Contributor

All of the ncc issues here have been resolved in 0.20.2.

14 and 15 should be fixed on the upgrade by default. If not, we can reinvestigate.

The now-node issues to do with over-filtering can be caught by providing the debugLog option to ncc to see logs like the above.

The filtering fix for 08 is to set filterAssetBase: path.resolve('/pkg/path') so that it doesn't use the process.cwd() filter base by default and instead knows that these root-level paths are ok /tmp/sdf or something I believe the path was.

@guybedford
Copy link
Contributor

Closing, but please let's reopen if any of these come up again.

ywg-jean pushed a commit to ywg-jean/now-builders that referenced this issue Jun 26, 2019
this includes the fixes for vercel/ncc#434
@ywg-jean
Copy link

ywg-jean commented Jun 26, 2019

hello @guybedford

I have updated my PR against now-builders to upgrade to [email protected]
unfortunately the tests are still failing: https://circleci.com/gh/zeit/now-builders/2662

08-assets

You comment suggests that the tests for assets (8 in both now-node and now-server) need to configure filterAssetBase to work properly.
Being unsure what the value for the option should be. I have tried both of the values that looked like path in the scope where ncc (entrypointPath and workPath) is called but neither resulted in a green test and I am way out of my depth there. revisiting everything I think I should pass path.resolve('workpath') but I get [Error: Rate limit exceeded] when I try to run the tests now :(
edit : after waiting for the limits to reset, path.resolve('workpath') fixed test 08.

09-include-files

In my understanding upgrading to [email protected] won't help.
I will try to rephrase the issue as I understand it, ncc doesn't know that it should package the templates directory.
Between 0.18.x and 0.20.x ncc started filtering paths more aggressively, and dynamically constructed paths can now be filtered out.
you advised changing the test but I am unsure I understand how it should be changed ... should it use path.join(__dirname ,...) as documented on https://edge.adonisjs.com/ and used in 08-assets or should it provide a different way to resolve the proper template path ?
edit : after waiting for the limits to reset using path.join(__dirname,'./templates') doesn't seem to be enough to fix this test.

14-stack-trace-ts

fails to build with the following error :

Cannot find name 'Error'.     

full trace :

ncc: Version 0.20.2
--
BUILD | Jun 26 2019 11:53:22:878 | /index.ts | ncc: Compiling file index.js
BUILD | Jun 26 2019 11:53:23:362 | /index.ts | ncc: Using [email protected] (ncc built-in)
BUILD | Jun 26 2019 11:53:23:516 | /index.ts | Error: [tsl] ERROR       TS6053: File '/tmp/1b3d251c/.build-utils/.builder/node_modules/@zeit/ncc/dist/ncc/loaders/typescript/lib/lib.esnext.d.ts' not found. 
[tsl] ERROR       TS2318: Cannot find global type 'Array'. 
[tsl] ERROR       TS2318: Cannot find global type 'Boolean'. 
[tsl] ERROR       TS2318: Cannot find global type 'CallableFunction'. 
[tsl] ERROR       TS2318: Cannot find global type 'Function'. 
[tsl] ERROR       TS2318: Cannot find global type 'IArguments'. 
[tsl] ERROR       TS2318: Cannot find global type 'NewableFunction'. 
[tsl] ERROR       TS2318: Cannot find global type 'Number'. 
[tsl] ERROR       TS2318: Cannot find global type 'Object'. 
[tsl] ERROR       TS2318: Cannot find global type 'RegExp'. 
[tsl] ERROR       TS2318: Cannot find global type 'String'. 
[tsl] ERROR in /tmp/1e2621cc/index.ts(4,17)       TS2304: Cannot find name 'Error'.     at compiler.close.n (evalmachine.<anonymous>:1:1539399)     at _promise0.then._result0 (eval at create (evalmachine.<anonymous>:1:273358), <anonymous>:13:1)     at <anonymous>

15-helpers

fails with

ncc: Using [email protected] (ncc built-in)
--
BUILD | Jun 26 2019 11:53:41:862 | /ts/index.ts | Error: [tsl] ERROR       TS6053: File '/tmp/2c9129f8/.build-utils/.builder/node_modules/@zeit/ncc/dist/ncc/loaders/typescript/lib/lib.esnext.d.ts' not found. 
[tsl] ERROR       TS2318: Cannot find global type 'Array'. 
[tsl] ERROR       TS2318: Cannot find global type 'Boolean'. 
[tsl] ERROR       TS2318: Cannot find global type 'CallableFunction'.
[tsl] ERROR       TS2318: Cannot find global type 'Function'. 
[tsl] ERROR       TS2318: Cannot find global type 'IArguments'. 
[tsl] ERROR       TS2318: Cannot find global type 'NewableFunction'. 
[tsl] ERROR       TS2318: Cannot find global type 'Number'. 
[tsl] ERROR       TS2318: Cannot find global type 'Object'. 
[tsl] ERROR       TS2318: Cannot find global type 'RegExp'. 
[tsl] ERROR       TS2318: Cannot find global type 'String'.     at compiler.close.n (evalmachine.<anonymous>:1:1539399)     at _promise0.then._result0 (eval at create (evalmachine.<anonymous>:1:273358), <anonymous>:13:1)     at <anonymous>

@styfle
Copy link
Member Author

styfle commented Jun 26, 2019

@guybedford I confirmed some of these are still bugs.

  • 08-assets: Works great, passes tests!
  • 09-include-files: This is still a mystery. This one actually isn't testing ncc. It's testing the config.includeFiles option to make sure users can add files even if ncc can't find them. Perhaps this should glob relative to the rootPath. Again, doesn't seem like an ncc issue but very strange that it shows up when bumping ncc version.
  • 14-stack-trace-ts: Definitely still a bug with ncc and typescript
  • 15-helpers: Definitely still a bug with ncc and typescript

@guybedford
Copy link
Contributor

guybedford commented Jun 26, 2019 via email

ywg-jean pushed a commit to ywg-jean/now-builders that referenced this issue Jun 26, 2019
this includes the fixes for vercel/ncc#434
@styfle
Copy link
Member Author

styfle commented Jun 28, 2019

@guybedford If I understand correctly, you are suggesting to get a copy of ts-loader from 0.18.5 and overwrite the existing ts-loader file in 0.20.2?

What about ts-loader.js.cache and ts-loader.js.cache.js? Do those need to be copied as well?

@ywg-jean
Copy link

@styfle I guess we were looking into this at the same time, I was writing :

@guybedford
I tried copying the ts-loader.js from a packaged 0.18.5 ncc to src/loaders/ts-loader.js
and ran the tests : it more or less exploded.
I then tried copying the src/loaders/ts-loader.js from https://github.com/zeit/ncc/blob/0.18.5/src/loaders/ts-loader.js and am running the tests right now (they take forever :( )
The last option I can think of is that you suggested to copy the ts-loader.js from a packaged 0.18.5 ncc into a packaged 0.20.2 ncc. We might be able to repackage that but is it really desirable to publish this as an official ncc version (for which the source code would basically be missing)

@guybedford
Copy link
Contributor

Yes, it's ts-loader.js.cache.

@styfle
Copy link
Member Author

styfle commented Jun 29, 2019

@guybedford I tried every combination of copying ts-loader, ts-loader.js.cache, and ts-loader.js.cache.js.

It always results in the same error:

ncc: Version 0.20.2
ncc: Compiling file index.js
ncc: Using [email protected] (ncc built-in)
Error: [tsl] ERROR TS6053: File '/tmp/619ce465/.build-utils/.builder/node_modules/@zeit/ncc/dist/ncc/loaders/typescript/lib/lib.esnext.d.ts' not found.
[tsl] ERROR
      TS2318: Cannot find global type 'Array'.
[tsl] ERROR
      TS2318: Cannot find global type 'Boolean'.
[tsl] ERROR
      TS2318: Cannot find global type 'CallableFunction'.
[tsl] ERROR
      TS2318: Cannot find global type 'Function'.
[tsl] ERROR
      TS2318: Cannot find global type 'IArguments'.
[tsl] ERROR
      TS2318: Cannot find global type 'NewableFunction'.
[tsl] ERROR
      TS2318: Cannot find global type 'Number'.
[tsl] ERROR
      TS2318: Cannot find global type 'Object'.
[tsl] ERROR
      TS2318: Cannot find global type 'RegExp'.
[tsl] ERROR
      TS2318: Cannot find global type 'String'.
    at compiler.close.n (evalmachine.<anonymous>:1:1539399)
    at _promise0.then._result0 (eval at create (evalmachine.<anonymous>:1:273358), <anonymous>:13:1)
    at <anonymous>

@guybedford
Copy link
Contributor

Weird, as that should be identical to the 0.18 behaviour... sure I will take a look first thing Monday.

@guybedford
Copy link
Contributor

These are all fixed now in the latest ncc. Will continue to track the TypeScript workaround in #435.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority Important issue or pull request to fast-track
Projects
None yet
Development

No branches or pull requests

3 participants