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: allow transformation of typescript files in node_modules #1385

Merged
merged 8 commits into from
Feb 12, 2020
Merged

fix: allow transformation of typescript files in node_modules #1385

merged 8 commits into from
Feb 12, 2020

Conversation

helmutschneider
Copy link
Contributor

@helmutschneider helmutschneider commented Feb 12, 2020

Fixes #1294 and possibly some of #1343

Most of this PR was derived from the implementation in ts-loader and a bunch of manual debugging. See #1381 for another possible solution.

Any comments are appreciated, especially in regards to testing. To make a meaningful E2E test we probably need a mocked package written in TS.

@coveralls
Copy link

coveralls commented Feb 12, 2020

Pull Request Test Coverage Report for Build 4012

  • 6 of 10 (60.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 90.135%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/compiler.ts 6 10 60.0%
Totals Coverage Status
Change from base Build 4009: -0.3%
Covered Lines: 1074
Relevant Lines: 1134

💛 - Coveralls

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 12, 2020

hi, thank you for your PR. Would you please add tests for your changes ? You can take the example tests from #1381. Imo this solution won't affect to the performance as much as #1381.

I think #1381 can be closed in favor of this PR. We have #1146 to try out Program.

@helmutschneider
Copy link
Contributor Author

Sure, do you have any suggestion for how we mock a node package with typescript files in it? I can think of two solutions:

  • We write the files manually to disk in the test method
  • We open a real repository with some TS in it (preferrably with the same author as ts-jest)

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 12, 2020

Sure, do you have any suggestion for how we mock a node package with typescript files in it? I can think of two solutions:

  • We write the files manually to disk in the test method
  • We open a real repository with some TS in it (preferrably with the same author as ts-jest)

I think the 1st solution is good for unit test and the 2nd solution is good with e2e test.

I have a branch which includes a monorepo to test #1381 . You can check that branch if you want to include the monorepo tests. Simply run npm run test:monorepo on that branch it will trigger tests for the whole folder __monorepos__.

@helmutschneider
Copy link
Contributor Author

Thanks, I stole the example test from your branch. However, it doesn't seem to run during an ordinary npm test. I'm not sure if that's intended or not.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 12, 2020

Oh you just need to add npm run test:monorepo to travis.yml, npm test only runs test:unit and test:e2e. Monorepo tests are actually run with test:external

@helmutschneider
Copy link
Contributor Author

Ok, done. Appveyor doesn't seem to like npm run test:monorepo. Should we only run monorepo tests on travis?

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 12, 2020

AppVeyor runs on Windows. It's slower than Travis because Travis is running on Linux. I think monorepo test on Travis is enough 👍

@ahnpnl ahnpnl requested a review from kulshekhar February 12, 2020 15:19
@kulshekhar
Copy link
Owner

If it's erroring out on appveyor, wouldn't also potentially cause problems for Windows users?

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 12, 2020

I have checked the log of AppVeyor it looked to me was the problem with the script to execute external project test, not ts-jest issue imo.

jest '/test/index.spec.ts' -> this seems to not valid path for windows. We have an open issue to try out Travis Windows, perhaps we can try adding monorepo test first for Travis Windows since Travis seems to be faster than AppVeyor somehow. I’ve tried to run unit tests and e2e tests with Travis Windows but there are some problems with snapshots so I think we can try with monorepo tests there first.

@kulshekhar
Copy link
Owner

ok, it's all good then!

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 12, 2020

@helmutschneider all good to go once you merge master :)

@helmutschneider
Copy link
Contributor Author

@ahnpnl Do you want me to squash too?

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 12, 2020

Oh it’s ok, I can do squash and merge once CI finishes.

@ahnpnl ahnpnl merged commit 814405e into kulshekhar:master Feb 12, 2020
@anbraten
Copy link

Do you plan to release this fix soon?

@kulshekhar
Copy link
Owner

I'll do that today

@kulshekhar
Copy link
Owner

done

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.

ts files in node_modules is not transformed
5 participants