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

NCC fails to run on the first time, but succeeds on the second run. #208

Closed
paulogdm opened this issue Jan 2, 2019 · 11 comments
Closed

Comments

@paulogdm
Copy link
Member

paulogdm commented Jan 2, 2019

Greetings!
I have searched for issues related to the one me and @mattapperson are facing, but I have failed to find it. Please advise if this is a known issue.

Summary

Monorepo available here. We can successfully run it locally, but building with cd app1 && ncc build index.ts fails when executed for the first time. However, executing it again succeeds!

Step By Step

git clone https://github.com/mattapperson/now-issue-example
cd now-issue-example
cd app1 && npm i
ncc build index.ts
ncc build index.ts

Any clues?

@paulogdm paulogdm changed the title Monorepo problem - Can't find package NCC fails to run on the first time, but succeeds on the second run. Jan 2, 2019
@mattapperson
Copy link

The example has been simplified further. Steps are now:
git clone https://github.com/mattapperson/now-issue-example
cd now-issue-example
npm i
ncc build index.ts
ncc build index.ts

@mattapperson
Copy link

mattapperson commented Jan 2, 2019

Please note the new example shows that this is not a Monorepo issue. Simply a project with directories issue...

@rauchg
Copy link
Member

rauchg commented Jan 2, 2019

I wonder if it's related to our new caching feature.

@guybedford
Copy link
Contributor

So the original error is the valid error in the following case:

app1/node_modules/io-ts
app1/index.ts -> ../shared/foo.ts
shared/foo.ts -> io-ts

"io-ts" can't be resolved from "shared/foo.ts" so there is an error thrown.

The fact that it works the second time is due to the cache exactly storing the resolution somehow, which sounds like a Webpack 5 bug here to me as the cache shouldn't be using the stored resolution for "io-ts" where it was a known failure.

These sorts of bugs are to be expected I suppose with caching work - error caching is very much a standard edge case.

//cc @sokra

Note the fix of the error is to move the node_modules down out of app1 so that it is shared for both, or to ensure a node_modules exists in shared with the correct dependency.

@mattapperson
Copy link

Please note the error also exists when there is just one package.json in root
mattapperson/now-issue-example@032e8f0

@guybedford
Copy link
Contributor

@mattapperson on that commit I'm getting the build working fine with and without caching.

@sokra
Copy link
Member

sokra commented Jan 5, 2019

@johnnyreilly this seem to be related to the way ts-loader reports errors/warnings to webpack. They are not attached to the Module, but to the Compilation instead. For caching errors must be attached to the Modules, best using emitWarning/Error from loader context. I'm unsure if this is doable since typescript is a whole program compilation. It even more difficult in this case since errors occur here in a module that is only typechecked but not compiled by webpack (it's a type-only import).

Maybe it makes more sense to run ts-loader in transpile-only mode and run the typechecking separately?

@johnnyreilly
Copy link

johnnyreilly commented Jan 5, 2019

Hey @sokra

For caching errors must be attached to the Modules, best using emitWarning/Error from loader context.

Is this a change in behaviour with webpack 5? I believe ts-loader has always used a combination of the module and the compilation to report errors:

https://github.com/TypeStrong/ts-loader/search?q=errors.push&unscoped_q=errors.push

i.e. either compilation.errors.push(...errorsToAdd) or module.errors.push(...formattedErrors)

Could you point me to an example that illustrates how to use emitWarning/Error? I haven't previously encountered either of those. Are there limitations when using them? Can I use them in the afterCompile hook?

I'm unsure if this is doable since typescript is a whole program compilation. It even more difficult in this case since errors occur here in a module that is only typechecked but not compiled by webpack (it's a type-only import).
Maybe it makes more sense to run ts-loader in transpile-only mode and run the typechecking separately?

That's generally my advice when using ts-loader anyway for performance reasons. See: https://github.com/TypeStrong/ts-loader#faster-builds

@sokra
Copy link
Member

sokra commented Jan 8, 2019

Is this a change in behaviour with webpack 5? I believe ts-loader has always used a combination of the module and the compilation to report errors:

This is not new. As I understand ts-loader it attaches a afterCompile plugin to report the errors/warnings from the typescript compiler.

https://github.com/TypeStrong/ts-loader/blob/e3d6367fe536a1d23d20a4b59e1cceef3ecabdb9/src/instances.ts#L294-L297

This works fine as long ts-loader was executed at least once. Without persistent caching this is always true when a typescript file is in the project. With persistent caching it can (and will) happen that no typescript file was changed since last run and all typescript modules can be restored from cache. In this case ts-loader is not executed (it's not even required) and the afterCompile plugin is not attached.

Errors pushed to module.errors are stored and restored while using persistent caching. emitError from the loader context calls module.errors.push so you are also fine doing the same.

Errors pushed to compilation.errors are not cached. And as the afterCompile plugin is not added when no typescript file is compiled, they vanish in this case. Note that loaderContext._compilation is a private API, so such problems are expected and ts-loader should be handled like a plugin regarding breaking changes in webpack.

There are multiple options to solve this:

Always add errors/warnings to a module and avoid compilation.errors. For type only imports you need to add the errors to the referencing modules.

Always add the plugin that add the errors to the compilation. This means it would be required to add a ts-loader.Plugin to the plugins array, which registers the afterCompile plugin.

@johnnyreilly
Copy link

johnnyreilly commented Jan 8, 2019

Thanks for responding @sokra .

Is this a change in behaviour with webpack 5? I believe ts-loader has always used a combination of the module and the compilation to report errors:

This is not new. As I understand ts-loader it attaches a afterCompile plugin to report the errors/warnings from the typescript compiler.

https://github.com/TypeStrong/ts-loader/blob/e3d6367fe536a1d23d20a4b59e1cceef3ecabdb9/src/instances.ts#L294-L297

Exactly right.

emitError from the loader context calls module.errors.push so you are also fine doing the same.

Cool - I wasn't aware of that API.

There are multiple options to solve this:

Always add errors/warnings to a module and avoid compilation.errors. For type only imports you need to add the errors to the referencing modules.

So it's safe to do this: module.errors.push(...formattedErrors)
But we shouldn't do this: compilation.errors.push(...formattedErrors)

Always add the plugin that add the errors to the compilation. This means it would be required to add a ts-loader.Plugin to the plugins array, which registers the afterCompile plugin.

If using this approach is the compilation.errors.push(...formattedErrors) approach safe with this option then? Could you point me to an example of this sort of registration please? I won't necessarily go with that approach but I'd like to understand.

@guybedford
Copy link
Contributor

This was resolved in #210.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants