Fix: Increase the instance version when new root files are found

[davazp]

This is a minimal fix for #943.

When a new root file is processed, ensure that the instance version is increased.

It is an alternative to the bigger PR #945.

[davazp]

This fixes the problem by correctly allowing files not covered by tsconfig.json.

There is no conflict with any other option and ts-loader behaves as it used to.

@johnnyreilly, @andrewbranch

[johnnyreilly]

Heya, thanks for this!

It looks like this tweak may be breaking the nodeModulesMeaningfulErrorWhenImportingTs test.

Essentially with the change this code is never executed:

https://github.com/TypeStrong/ts-loader/blob/6e955861994f04943e57303f2e2beb77670d9ede/src/index.ts#L164-165

Interestingly, actually importing ts from node_modules (a feature I feel that has questionable value but that people wanted) still seems fine: https://github.com/TypeStrong/ts-loader/tree/master/test/execution-tests/allowTsInNodeModules

Can you confirm why the meaningful error message code isn't being executed now please? I've a feeling it may be valid behaviour that this is the case but I'd like to know for sure.

If there's good reasons to go with this behaviour then we should think about removing code around the meaningful error message and it's associated comparison test:

https://github.com/TypeStrong/ts-loader/tree/master/test/comparison-tests/nodeModulesMeaningfulErrorWhenImportingTs

But before we do that, let's be sure this is intentional. Thanks!

PS more information here:

• https://github.com/TypeStrong/ts-loader/blob/master/README.md#allowtsinnodemodules-boolean-defaultfalse
• Guidance on shipping ts within node_modules microsoft/TypeScript#12358
• Add allowTsInNodeModules option for importing .ts files from node_modules. #773

cc @aelawson

[davazp]

Ok, this is interesting. It seems very similar to the bug I was trying to fix! Only that in my situation, instead of an explicit node_modules directly, I was using yarn workspaces.

I guess I can normalize the path to remove symlinks and check for node_modules as well.

[johnnyreilly]

It doesn't look like your change has resolved the issue? Was it intended to?

I think my original questions still stand; is this something that we should be concerned about?

[davazp]

It doesn't look like your change has resolved the issue? Was it intended to?

That is strange! It works for me locally. I'll try to find out more.

I think my original questions still stand; is this something that we should be concerned about?

I think we should. My changes allow you to compile files that are under node_modules even if allowTsInNodeModules is falsed

[davazp]

Silly mistake. Wrong comparison... ! It worked locally because it updated the snapshot 😄

I updated the PR.

Should we also check for /node_modules/ . (for whatever separators the OS has?) . Not likely but a path like abcnode_modulesxyz would match this and be considered node_modules/. I leave it out for another PR maybe.

[johnnyreilly]

Yeah - I think that's an edge case. I'm happy with how things are now I think. Do you want to update the package.json and CHANGELOG.md please?

[johnnyreilly]

I'm afraid we already have 6.0.3 published... 6.0.4?

[johnnyreilly]

Release going out now - thanks for your help!

https://github.com/TypeStrong/ts-loader/releases/tag/v6.0.4

[davazp]

Awesome. Thank you @johnnyreilly and @andrewbranch for your help, responsiveness and the nice test suite 🙂