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

Tries to compile sources that are not supposed to be loaded #267

Closed
wclr opened this issue Aug 20, 2016 · 41 comments
Closed

Tries to compile sources that are not supposed to be loaded #267

wclr opened this issue Aug 20, 2016 · 41 comments

Comments

@wclr
Copy link

wclr commented Aug 20, 2016

Webpack 2 and TS2 beta
ts-loader tries to compile even source that should not be loaded by webpack. Is it expected behaviour?

@blakeembrey
Copy link
Member

Can you give more information? I can't tell you without seeing an example, but most like it's not compiling it - it's using it for type information and that would be expected.

@wclr
Copy link
Author

wclr commented Aug 21, 2016

I serve files mostly in /client directory (by ts-loader), and I have ts file say server-somthing.ts in /server (that is loaded using ts-node) - this file is not shared with client part.

And say there is a mistake in server-somthing.ts.

But when webpack lanunches it error with compile error in server-somthing.ts. To get rid of it I have to limit files in tsconfig and if I use awesome-typescript-loader instead of ts-loader (just replace for it in webpack.confg) it works ok - doesn't try to compile server-somthing.ts.

@Vloz
Copy link

Vloz commented Sep 17, 2016

Same things for me:
ts-loader is giving error from file not used in the entry, but are part of the file tree.
I have 5 different ts entry path in the same filetree, and it looks like they are parsing each-other's files... Wonder if it doesnt overslow the build speed... @_@

@timocov
Copy link
Contributor

timocov commented Oct 12, 2016

@blakeembrey Hi, guys! This issue prevents us from using ts-loader. We've made a simple example so you can reproduce it.
This example contains tests folder with additional dependencies in package.json which are not to be installed to build the base project.
just unpack it, and run

npm install
./node_modules/.bin/webpack

You'll get an error in tests/src/test.ts which is not required to build the base project:
(1,25): error TS2307: Cannot find module 'jquery'.

Example project: ts-loader-fail.zip

@johnnyreilly
Copy link
Member

Hi @timocov,

Thanks for the repro - that's very helpful and we're always grateful when someone can provide a clear example of something not working as they expect.

We're open to a receiving a PR that will resolve this 🌷

@ezhukovskiy
Copy link

@johnnyreilly Hi, this issue is solved in awesome-typescript-loader.
The problem is that after-compile in ts-loader looks for errors in all files from tsconfig.json.
Look at how it is done in awesome-typescript-loader:
https://github.com/s-panferov/awesome-typescript-loader/blob/master/src/instance.ts#L396
It seems that they gather all files the loader was called for and checks them in after-compile.
I believe that you will be able to understand more from their code knowing how it is done in ts-loader.

@johnnyreilly
Copy link
Member

Thanks for the clarification @eugenezee - very helpful. PRs welcome!

@johnnyreilly
Copy link
Member

johnnyreilly commented Oct 17, 2016

Hi @EugeneZee,

I took a quick look at the awesome-typescript-loader approach and it wasn't obvious to me where they gather just the files the loader was called for. Could you clarify that for me please? I'm interested in using this approach; I just need to understand exactly how it works... This looks like it might be what you're talking about?

https://github.com/s-panferov/awesome-typescript-loader/blob/master/src/instance.ts#L443

But without diving deep into the source of atl it's not clear to me where the information about which files the loader was called for comes from...

@ezhukovskiy
Copy link

ezhukovskiy commented Oct 20, 2016

@johnnyreilly Hi,
As soon as I understand it stores files when it is called to compile it:

  1. In a "global" array that will be used to check errors:
    https://github.com/s-panferov/awesome-typescript-loader/blob/master/src/index.ts#L61
  2. In an instance array that will be used to create phantomImports:
    https://github.com/s-panferov/awesome-typescript-loader/blob/master/src/index.ts#L58

On after-compile it checks list [1] for errors and gathers all the files in the project like ts-loader, but then it checks where the compiled files list [2] contains them or not. If a file is not in the list it puts it in phantomImports:

https://github.com/s-panferov/awesome-typescript-loader/blob/master/src/instance.ts#L443

He uses phantomImports to find appropriate declaration (d.ts) files. He puts them in compilation.assets and compilation.fileDependencies.

@johnnyreilly
Copy link
Member

Thanks for the write up @EugeneZee - very helpful. This is on my radar but I'm not sure when I'll get to it; in the meantime PRs are always welcome!

@johnnyreilly
Copy link
Member

Related to #54

nilshartmann added a commit to nilshartmann/spring-petclinic that referenced this issue Oct 26, 2016
Due to a bug in ts-loader (TypeStrong/ts-loader#267) tests will always be compiled
even from webpack. So I've moved them for now to a separate folder.

TODO: need to re-check why type declarations from npm (@types) does not work.
@wclr
Copy link
Author

wclr commented Nov 28, 2016

Are there recommendation how it could be worked around?

@ezhukovskiy
Copy link

@whitecolor we ought to use awesome-webpack-loader for tests sub-project and ts-loader for the base project, which is inconvenient

@johnnyreilly
Copy link
Member

The best workaround is a pull request 😄 More seriously this isn't an issue I've encountered in the various projects I've worked on. If it helps I tend to avoid having code that would "collide"

@s-panferov
Copy link

ts-loader tries to compile even source that should not be loaded by webpack. Is it expected behaviour?

I think that it's OK in some cases. We should expect webpack+tsc to produce the same compilation results as just pure tsc run. See my comment here.

@ezhukovskiy
Copy link

ezhukovskiy commented Nov 29, 2016

I think that it's OK in some cases.

I'm OK with new behavior as an option "bundle", but it is critical to have this option at least to keep tests in the project repository which is a common approach.

@johnnyreilly
Copy link
Member

but it is critical to have this option at least to keep tests in the project repository which is a common approach

This is an example of a toy project of my own which does exactly that but doesn't appear to suffer from this issue:

https://github.com/johnnyreilly/proverb-signalr-react?files=1

@timocov
Copy link
Contributor

timocov commented Nov 29, 2016

@johnnyreilly that's because you have all dependencies in root package.json file. So, if you don't need to run tests, you'll install a lot of unnecessary deps.

@johnnyreilly
Copy link
Member

This is not to belittle the issue you are facing - rather to provide an example of an approach which seems to work (at least for some use cases)

@johnnyreilly
Copy link
Member

that's because you have all dependencies in root package.json file. So, if you don't need to run tests, you'll install a lot of unnecessary deps.

That's certainly true but they won't be included when I bundle for release. As such I'm OK with that (whilst remaining open to better solutions should they present themselves)

@timocov
Copy link
Contributor

timocov commented Nov 29, 2016

I think the problem is about a lot of time which spend on download unnecessary deps, not about what will be in result bundle.

There is workaround, but only for typescript 2.1 - inherited projects.

@wclr
Copy link
Author

wclr commented Nov 29, 2016

There is workaround, but only for typescript 2.1 - inherited projects.

What do you mean by inherited projects, extends? How does it help?

@timocov
Copy link
Contributor

timocov commented Nov 29, 2016

https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#configuration-inheritance

You can make base project file which includes compiler options and some other settings (include for example) and another one (which extends first) which will exclude your test folder. Second config you can use for build by webpack and the first for development (for example)

@timocov
Copy link
Contributor

timocov commented Nov 29, 2016

Possible it helps you exclude tests files (which placed nearly with source files) too (by using specific exclude parameter), but I don't check it.

@quantuminformation
Copy link

I find that using this loader also tries to compile files that are in the tsconfig exclude option:

https://github.com/quantumjs/solar-popup/blob/master/tsconfig.json#L8

@StephanBijzitter
Copy link

I also just walked into this issue, where one project with failing types is now causing the other 3 projects in the same repository to also show errors when compiling. Yes, this is the same behaviour as running tsc manually, but with webpack I would expect only the files that are required/imported to be transpiled.

@quantuminformation
Copy link

Possibly related to

#544

@tkrotoff
Copy link

What about ignoring the files and include parts of tsconfig.json and only compile the files known to webpack? This would simplify everything.

This is what rollup-plugin-typescript2 does, it only checks for the compilerOptions part of tsconfig.json (see #646).

@johnnyreilly
Copy link
Member

We'd be open to a pull request if you fancied implementing this @tkrotoff.

@tkrotoff
Copy link

@johnnyreilly first we need to be sure there no need for files and include parts from tsconfig.json. Is there any counter-example? And also it will break compatibility.

@johnnyreilly
Copy link
Member

@tkrotoff - if it goes in it would be behind a flag. ts-loader aims to be a drop in replacement for tsc as much as is practical. Since this would deviate from that I'd want this to be "opt-in" behaviour; not default.

To your question: I believe files and include are both optional.

@tkrotoff
Copy link

To your question: I believe files and include are both optional.

There is a misunderstanding. Of course files and include are optional in tsconfig, not talking about that.

What I suggest is to totally ignore them inside ts-loader: ts-loader would only compile what webpack provides, nothing more.

@johnnyreilly
Copy link
Member

Ok I see. As long as you're ok with this being opt-in rather than default behaviour then I say start implementing in a PR and use that as a basis for conversation. Don't worry about failing tests or anything- just hack about, raise a PR and talk to me. Let's see what happens! 😄

@tkrotoff
Copy link

@johnnyreilly

start implementing [...] use that as a basis for conversation

I prefer you to be sure that the idea is good and it's the proper way to handle things. I don't want to spend hours on a PR that ends up with "sorry, the idea is wrong, here is a valid use case where we need files/include from tsconfig.json".

For me it's clear:

  • files and include from tsconfig.json => tsc
  • webpack entry (and nothing else except compilerOptions) => ts-loader (the way rollup-plugin-typescript2 works)

In my opinion it should have been this way from the start. Currently ts-loader mix tsconfig.json files/include and webpack entries (+ other things hence the current issue) and it's a mess.

Ping @s-panferov any opinion on this?

@johnnyreilly
Copy link
Member

On the face of it seems reasonable.

I'd suggest start attempting it - it might be the case that doing so reveals "dragons". Or not. Purely from my own perspective I find it easier to think about code during implementation; hence my suggestion.

As always, @s-panferov views could be helpful. Just to be clear though, I would plan to put this behaviour behind a feature flag (loader option) at least initially. If it's clear that this can be inferred from the tsconfig.json then great. But I'm not counting on it.

@johnnyreilly
Copy link
Member

This should be resolved by @maier49's PR: #671

Hope to ship this with ts-loader 3.1.0

@Storyyeller
Copy link

Storyyeller commented Dec 20, 2017

I am still seeing this issue in ts-loader 3.2.0. It makes ts-loader and webpack literally unusable for us.

Webpack loaders are supposed to be called to handle the files you're building with webpack - not to spend minutes blindly processing every file in your repository and then blowing up because of unrelated issues.

Edit: For anyone else reading, you can work around this bug by specifying the option onlyCompileBundledFiles like so

use: [{loader: 'ts-loader', options: {onlyCompileBundledFiles: true}}]

I still find it astonishing that ts-loader is broken by default, but at least there's a workaround.

@johnnyreilly
Copy link
Member

johnnyreilly commented Dec 21, 2017

ts-loader intentionally mirrors the behaviour of tsc as closely as possible.

If tsc thinks your code is probably broken then ts-loader should say so too. We do this so there's not a disconnect between the editor / IDE experience and your build.

The onlyCompileBundledFiles is one of a number of flags that allows you to deviate behaviour from tsc and align closer with typical webpack behaviour.

ts-loader is broken by default

It's not, it's just not the behaviour you expected.

I am still seeing this issue in ts-loader 3.2.0. It makes ts-loader and webpack literally unusable for us.

To be clear: is it usable with the onlyCompileBundledFiles flag? Or not?

@Storyyeller
Copy link

Storyyeller commented Dec 21, 2017

I was under the impression that ts-loader is intended to be used with webpack. Why not follow the standard webpack behavior in that case?

To be clear: is it usable with the onlyCompileBundledFiles flag? Or not?

Yes. The issue is that there were a large number of other files in the repository which shouldn't be checked but which ts-loader checks by default. Setting the flag fixed this.

P.S. Sorry for the overly harsh language earlier. I understand that it is working as intended, I just can't understand why it is intended to behave like that, and I don't think the default behavior makes sense, which makes it a bug from my perspective.

@johnnyreilly
Copy link
Member

I was under the impression that ts-loader is intended to be used with webpack. Why not follow the standard webpack behavior in that case?

It is intended to be used with webpack. However we want the typical compiler behaviour to tie up with ts-loader as much as possible.

Partly to give a good editor / IDE experience. Partly because webpack is not the only game in town. People may move to webpack and away from webpack as a build choice. There can be good reasons to do either. Ideally that should be a frictionless experience. We don't want people to have to use webpack / ts-loader - it should be a choice. The minute we have default behaviour that deviates from tsc then we've started down that unfortunate road.

It might not be the choice you prefer but it's not something that's likely to change, for the reasons given. That said, I'm glad onlyCompileBundledFiles gives you what you need. That's fantastic!

It's possible (probable!) that our docs could be clearer around this. If you wanted to submit a docs PR to improve things I'd happily take a look. All the best : 🌻

@ericnoguchi
Copy link

ericnoguchi commented Dec 31, 2017

Hello,

Yes this issue caught me off guard as well
And for the record awesome-typescript-loader is also doing the same thing but each have a different workaround

  • onlyCompileBundledFiles fixes the issue to ts-loader
  • on the other hand fixing the issue for awesome-typescript-loader seems to be adding a "files": [],in tsconfig.json but ts-loader and TSC complains that TS18002: The 'files' list in config file 'tsconfig.json' is empty.
{
    "compilerOptions": {
        "sourceMap": true,
        "noImplicitAny": true,
        "module": "esnext",
        "target": "esnext",
        "allowJs": false,
        "checkJs": false
    },
    "files": [],
    "exclude": [
        "node_modules",
        "dist",
    ]
}

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

No branches or pull requests