Skip to content

Incorrect file list maintenance causes compiler error in particular cases #71

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

Closed
oryol opened this issue Oct 1, 2015 · 14 comments
Closed
Labels

Comments

@oryol
Copy link

oryol commented Oct 1, 2015

Prerequisites:

  • There are several TypeScript files in the project (as separated external modules because I want to use multiple bundles)
  • There are some npm based dependencies

The flow which causes to error:

  • When fist file are processed by ts-loader - typescript tries to load dependent npm modules. It checks package.json of the appropriate module (it can contain "typings" with direct reference to .d.ts).
  • TypeScript calls moduleResolutionHost.fileExists and readFile. And it's processed (in the ts-loader) through servicesHost.getScriptSnapshot (which is incorrect I think). As result - package.json is added to files array.
  • When next file is loaded using ts-loader (but in the same webpack instance of course) - TypeScript compiler calls serviceHost.getScriptFileNames and it returns package.json files which causes build fail without any understandable error, only "TypeScript emitted no output", because ts-loader doesn't show error messages from getCompilerOptionsDiagnostics and there is no error for specific source file, just for input file list (it contains json files).

As a temporary fix (obiously, it's better to decouple module resolution host from the service host, it's just a workaround) I fixed ts-loader/index.js (directly in the my node_modules) using following code in the getScriptFileNames:

return Object.keys(files).filter(function(f) { return !!f.match(/\.tsx?$/); });

It causes that json files are not returned for TypeScript input but still are saved in the files array (maybe it's ok to cache package.json in this array, I'm not sure actually).

Also I suggest to add getCompilerOptionsDiagnostics to error list in some way. Because it's very hard to understand what is causing error in some cases.

@jbrantly
Copy link
Member

jbrantly commented Oct 1, 2015

Thanks for the detailed report!

I think you're probably right about not using getScriptSnapshot for the moduleResolutionHost. It was done that way because that's how TS does it automatically if you don't override resolveModuleNames but since we are it probably doesn't make sense.

I'm not sure what you meant by ts-loader not showing error messages from getCompilerOptionsDiagnostics though. It should: https://github.com/TypeStrong/ts-loader/blob/master/index.ts#L350

@oryol
Copy link
Author

oryol commented Oct 1, 2015

I see. But it's invoked only when compiler is created. In my case the first file compiled successfully, but 2nd file compilation fails (actually, file list was wrong on this stage, it contained json files).

As result error messages says only:

Module build failed: Error: Typescript emitted no output for ...
    at Object.loader (...\node_modules\ts-loader\index.js:434:15)

After I have changed getScriptFileNames (added filtering by file extension) - everything compiled successfully. I have checked in the debugger - at least after languageService.getEmitOutput for the 2nd file (unfortunately I didn't discovered when exactly it appears) getCompilerOptionsDiagnostics returns error messages about json files.

If it's needed I can create minimal reproducible example for this case.

@jbrantly
Copy link
Member

jbrantly commented Oct 1, 2015

I don't think it's needed (yet). If I can't reproduce I'll let you know. What version of TypeScript are you using?

@oryol
Copy link
Author

oryol commented Oct 1, 2015

1.6.2

@alexgorbatchev
Copy link

I can confirm that proposed solution fixes the TypeScript emitted no output error.

Hash: 62ab4fd0eea84a83b566
Version: webpack 1.12.2
Time: 1982ms
     Asset       Size  Chunks             Chunk Names
index.html  292 bytes          [emitted]
chunk    {0} main.62ab4fd0eea84a83b566.js (main) 2.71 kB
     + 3 hidden modules

ERROR in ./app/client/index.tsx
Module build failed: Error: Typescript emitted no output for xxx/app/client/index.tsx
    at Object.loader (xxx/node_modules/ts-loader/index.js:408:15)
 @ multi main
webpack: bundle is now VALID.

@jbrantly
Copy link
Member

jbrantly commented Oct 6, 2015

I can't duplicate this after all :( I'm probably missing a setting somewhere. Could you provide a minimal example, including tsconfig.json and ideally webpack.config.js?

@jbrantly
Copy link
Member

jbrantly commented Oct 6, 2015

I put my attempt at duplicating this in #75 in case it helps. I basically took the existing nodeResolution test and added an additional dependency so that the loader was invoked twice (once for app.ts and once for b.ts).

I verified that getScriptFileNames was being called and that it did indeed contain the package.json file but I wasn't able to get an error.

@alexgorbatchev
Copy link

@jbrantly I emailed project sample to your public email.

@jbrantly
Copy link
Member

jbrantly commented Oct 6, 2015

Ah-ha! You need noEmitOnError enabled to duplicate. Thanks @alexgorbatchev

@alexgorbatchev
Copy link

Very cool @jbrantly, kudos for figuring this out! Thank you.

@oryol
Copy link
Author

oryol commented Oct 6, 2015

Good to know that it has been fixed!
Just to know - when are you going to release it to the npm?

jbrantly added a commit that referenced this issue Oct 6, 2015
@jbrantly
Copy link
Member

jbrantly commented Oct 6, 2015

In a day or two, probably. There are a couple other issues I'd like to get into the same release, and the PR still needs signing off.

@oryol oryol closed this as completed Oct 6, 2015
jbrantly added a commit that referenced this issue Oct 6, 2015
@jbrantly
Copy link
Member

jbrantly commented Oct 7, 2015

This was published in v0.5.6.

Also, thanks @oryol and @alexgorbatchev for reporting this and helping debug/fix it, and apologies for the delay in getting it out.

@alexgorbatchev
Copy link

Fantastic, thank you so much!

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

No branches or pull requests

3 participants