-
Notifications
You must be signed in to change notification settings - Fork 292
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
Comments
08-assetsThe debugging information on this test shows:
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
I'm not sure I follow this? I looked into the issue further and it really does come down to: do we want any The reason this becomes a problem is that we have other analysis expressions which can also resolve to dirname, eg 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-traceAgain, 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-helpersAgreed this sounds like a similar case. |
I managed to trace 14 and 15 down to #435. |
For 08 - if I navigate to the application directly, I can see the server outputting exactly |
We finally traced down 08 to the following filtering issue by building with
Created #440. |
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 The filtering fix for 08 is to set |
Closing, but please let's reopen if any of these come up again. |
this includes the fixes for vercel/ncc#434
hello @guybedford I have updated my PR against now-builders to upgrade to [email protected] 08-assetsYou comment suggests that the tests for assets (8 in both now-node and now-server) need to configure 09-include-filesIn my understanding upgrading to 14-stack-trace-tsfails to build with the following error :
full trace :
15-helpersfails with
|
@guybedford I confirmed some of these are still bugs.
|
Thanks @styfle - 14,15 seems like another builtin TypeScript bug and I
believe I know the issue. Will aim to post a patch in the next few days.
The workaround can be to copy the dist/ncc/loaders/ts-loader.js file in the
ncc 18 build over the 0.20 build and publish that as this file is the same
bit just an internal build issue.
…On Wed, 26 Jun 2019 at 18:44, Steven ***@***.***> wrote:
@guybedford <https://github.com/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 verison.
- 14-stack-trace-ts: Definitely still a bug with ncc and typescript
- 15-helpers: Definitely still a bug with ncc and typescript
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#434>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAESFSSANZY3MMQRX7NC67DP4OMG7ANCNFSM4HZNVPWA>
.
|
this includes the fixes for vercel/ncc#434
@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 |
@styfle I guess we were looking into this at the same time, I was writing : @guybedford |
Yes, it's |
@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:
|
Weird, as that should be identical to the 0.18 behaviour... sure I will take a look first thing Monday. |
These are all fixed now in the latest ncc. Will continue to track the TypeScript workaround in #435. |
The
now-builders
repo has 4 failing test fixtures and most of them appear to be regressions inncc
.Update: Tracking issues -
Steps to reproduce
Please clone the repo and install dependencies:
I went through each fixture and some fail differently with the CLI vs API.
08-assets
assets
paths.09-include-files
glob
instead of emittingassets
so hold off on changing this one for now.14-stack-trace-ts
15-helpers
I think the TypeError can be ignored and probably fixing test 14 will fix 15.
The text was updated successfully, but these errors were encountered: