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

fix: filter import errors and close #653 #853

Closed
wants to merge 1 commit into from

Conversation

rolandjitsu
Copy link

Fix for #653 (following #653 (comment)). Note that I did not add any tests as I was unsure where to add the tests.

@johnnyreilly
Copy link
Member

johnnyreilly commented Oct 8, 2018

Could you share a minimal repo that demonstrates this issue? I can use that as a basis for an execution test.

Also, can you confirm that you experience this regardless of the value of the emitDecoratorMetadata flag? Thanks!

@rolandjitsu
Copy link
Author

Let me setup that 😄

@rolandjitsu
Copy link
Author

rolandjitsu commented Oct 8, 2018

@johnnyreilly I just setup tsl-import, but I also just found that you can already disable these warnings with:

stats: {
  warningsFilter: /export .* was not found in/
}

But if you switch to react-scripts-ts branch, you can still see the same kind of error, but different message. I'm trying to track it down as I'm not sure if this is coming from ts-loader.

If I change transpileOnly to false in their webpack config, everything compiles just fine.

@johnnyreilly
Copy link
Member

But if you switch to react-scripts-ts branch, you can still see the same kind of error, but different message. I'm trying to track it down as I'm not sure if this is coming from ts-loader.

It'll likely be coming from: https://github.com/Realytics/fork-ts-checker-webpack-plugin

react-scripts-ts uses that for checking types and ts-loader for transpilation.

@rolandjitsu
Copy link
Author

rolandjitsu commented Oct 8, 2018

That's the thing, I'm not so sure about that as I removed the plugin from the webpack config and I still see this error.

For instance, in my example repo, if you checkout the react-scripts-ts-ejected branch and disable the transpile only option, all goes well.

It also seems strange that with transpileOnly: true it does show the error, but with transpileOnly: false it does not. If it were a different tool that is independent from ts-loader, it should always show the error.

@rolandjitsu
Copy link
Author

In any case, I don't think this PR will solve the issue anyway, so maybe we should close it?

@johnnyreilly
Copy link
Member

That's the thing, I'm not so sure about that as I removed the plugin from the webpack config and I still see this error.

react-scripts-ts is still webpack 3. I wonder if that's the reason?

@johnnyreilly
Copy link
Member

In any case, I don't think this PR will solve the issue anyway, so maybe we should close it?

If you're sure. I'm sorry you went to wasted effort though!

@rolandjitsu
Copy link
Author

rolandjitsu commented Oct 8, 2018

That's the thing, I'm not so sure about that as I removed the plugin from the webpack config and I still see this error.

react-scripts-ts is still webpack 3. I wonder if that's the reason?

I will try with w4 and see how that works out. Actually, the next tag on npm of create-scripts-ts is using webpack 4 (see react-scripts-ts-ejected/package.json#L61).

@rolandjitsu
Copy link
Author

Ok, I tracked down the error. Seems to be happening at webpack/lib/dependencies/HarmonyExportImportedSpecifierDependency.js.

@rolandjitsu rolandjitsu closed this Oct 8, 2018
@rolandjitsu
Copy link
Author

Could I ask what is different from what ts-loader generates as output with transpileOnly: true vs transpileOnly: false?

@johnnyreilly
Copy link
Member

Sure - there's a subtle difference in terms of produced code; but this only comes into play rarely; when emitDecoratorMetadata: true is in play and also I've an idea that imported enums also have a nuance. There may be more detail here:

https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#transpiling-a-single-file

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

Successfully merging this pull request may close these issues.

2 participants