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

Add allowTsInNodeModules option for importing .ts files from node_modules. #773

Merged
merged 10 commits into from May 7, 2018

Conversation

aelawson
Copy link
Contributor

@aelawson aelawson commented May 2, 2018

Summary:

This PR adds an allowTsInNodeModules option that will allow importing of .ts modules from a node_modules subdirectory / dependency. At the time of writing, this will function as a "whitelist" - the user must manually add the desired files(s) or module(s) to the files or include block of their tsconfig.json, respectively. Otherwise, they will receive an error that nothing has been emitted as before with additional text suggesting this option if they really need it.

Changes:

  • Added allowTsInNodeModules option.
  • Updated error message when a module is imported from node_modules without this option enabled.
  • Updated the nodeModulesMeaningfulErrorWhenImportingTs test outputs and have added a new allowTsInNodeModules test (with the option enabled) for TS 2.8.
  • Updated README to include this option.

As most recently discussed here:
#278

And also here:
#365
microsoft/TypeScript#12358

Andrew Lawson added 2 commits May 1, 2018 22:02
@johnnyreilly
Copy link
Member

Thanks for the thorough PR @aelawson! I'm getting failing tests on the build servers. It occurs to me that an execution test would likely work just as well here. Given that the comparison tests can be a world of pain, do you fancy trying putting together an execution test instead? I think that you can probably clone this one:

https://github.com/TypeStrong/ts-loader/tree/master/test/execution-tests/nodeResolution

And quickly tweak it into an execution test. Want to give it a whirl?

@aelawson aelawson force-pushed the aelawson/allow-node-modules branch from d04d5e2 to 84d7aa7 Compare May 2, 2018 19:38
@aelawson
Copy link
Contributor Author

aelawson commented May 2, 2018

@johnnyreilly Sure I'll change the test to be an execution test :) Yeah, looks like one of the failures was me forgetting to update the test output after a small change, d'oh.

Andrew Lawson added 2 commits May 2, 2018 22:35
Add tests for successful import of module and file.
Remove webpack output during karma tests.
@aelawson
Copy link
Contributor Author

aelawson commented May 3, 2018

@johnnyreilly I have updated the test to be an execution test - looks like builds are passing now. Let me know if you've got any other concerns!

@johnnyreilly
Copy link
Member

Nice - thanks for the hard work! I'll set aside some time to review it very soon!

README.md Outdated
Note that this option acts as a *whitelist* - any modules you desire to import must be included in
the `"files"` or `"include"` block of your project's `tsconfig.json`.

See: https://github.com/Microsoft/TypeScript/issues/12358
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we linkify this please? i.e. [https://github.com/Microsoft/TypeScript/issues/12358](https://github.com/Microsoft/TypeScript/issues/12358) I think

src/index.ts Outdated
filePath.indexOf('node_modules') !== -1
? '\nYou should not need to recompile .ts files in node_modules.\nPlease contact the package author to advise them to use --declaration --outDir.\nMore https://github.com/Microsoft/TypeScript/issues/12358'
: '';
let additionalGuidance: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we turn this into a ternary please? Something like:

const additionalGuidance = (!options.allowTsInNodeModules && filePath.indexOf('node_modules') !== -1)
    ? " By default, ts-loader will not compile .ts files in node_modules.\n" +
      "You should not need to recompile .ts files there, but if you really want to, use the allowTsInNodeModules option.\n" +
      "See: https://github.com/Microsoft/TypeScript/issues/12358"
    : "";

@johnnyreilly
Copy link
Member

Also, can we lose the comparison test now we have an execution test please?

Andrew Lawson added 2 commits May 3, 2018 11:21
@aelawson
Copy link
Contributor Author

aelawson commented May 3, 2018

@johnnyreilly Done. Just to double check on one thing: the remaining comparison test is the existing nodeModulesMeaningfulErrorWhenImportingTs test that checks for a meaningful error if a user hasn't enabled this option - you're okay with getting rid of that?

@johnnyreilly
Copy link
Member

Hey @aelawson,

I want to keep nodeModulesMeaningfulErrorWhenImportingTs please; that's still useful. Just wanted to get rid of the new comparison test since you've been able to come up with an execution test instead. I prefer execution tests but they're not always appropriate.

@aelawson
Copy link
Contributor Author

aelawson commented May 4, 2018

@johnnyreilly That comparison test has been deleted :P The only changes to comparison tests left in the PR are for the nodeModulesMeaningfulErrorWhenImportingTs and validateLoaderOptions tests :)

@johnnyreilly
Copy link
Member

Nice one! I'm not near a computer for the next half week or so. Once I am I'll look to merge and arrange a release. Thanks for your work!

@aelawson
Copy link
Contributor Author

aelawson commented May 4, 2018

Cool sounds good :)

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.

None yet

2 participants