Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Use typescript's tsconfig.json parsing functionality instead of JSON.parse #70

Merged
merged 5 commits into from
Jan 30, 2019
Merged

Use typescript's tsconfig.json parsing functionality instead of JSON.parse #70

merged 5 commits into from
Jan 30, 2019

Conversation

samuela
Copy link
Contributor

@samuela samuela commented Jan 29, 2019

JSON.parse differs from the semantics of typescript's parsing, as JSON.parse doesn't support comments. This PR switches to using typescript's provided parsing functionality for compatibility.

Tests pass with the exception of "tsPath should use the provided esModuleInterop option". For some reason, fancy.stub() doesn't seem to be working: The tests are still calling the unpatched version. I've tried stub() both on a global imported version of "../src/ts-node" and the "freshRequired" versions. Do let me know if I'm doing something wrong here...

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @samuela to sign the Salesforce.com Contributor License Agreement.

@samuela
Copy link
Contributor Author

samuela commented Jan 29, 2019

cc @jdxcode

@jdx
Copy link
Contributor

jdx commented Jan 29, 2019

off-hand, I'm not really sure why this isn't working. The freshRequire() bit is a little strange though, that shouldn't be necessary. I would try to do it the same way as before without trying to scope it to .add() maybe

@samuela
Copy link
Contributor Author

samuela commented Jan 29, 2019

@jdxcode freshRequire() was in the original test suite, so I just presumed that it served some useful purpose here. I can try without though.

@jdx
Copy link
Contributor

jdx commented Jan 29, 2019

apologies this turned into such a mess, I might be able to take a look later this week if nothing else

@samuela
Copy link
Contributor Author

samuela commented Jan 29, 2019

@jdxcode Here are the results when importing a single global ../src/ts-node and then stubbing that:

  1) tsPath
       should leave esModuleInterop undefined by default:

      AssertionError: expected 0 to equal 1
      + expected - actual

      -0
      +1
      
      at Context.withMockTsConfig.it (test/ts-node.test.ts:47:53)
      at Object.run (node_modules/fancy-test/lib/base.js:27:38)
      at Context.run (node_modules/fancy-test/lib/base.js:50:36)

  2) tsPath
       should use the provided esModuleInterop option:

      AssertionError: expected 0 to equal 1
      + expected - actual

      -0
      +1
      
      at Context.withMockTsConfig.it (test/ts-node.test.ts:60:53)
      at Object.run (node_modules/fancy-test/lib/base.js:27:38)
      at Context.run (node_modules/fancy-test/lib/base.js:50:36)

@samuela
Copy link
Contributor Author

samuela commented Jan 30, 2019

Ok, I tried stubbing out the function with proxyquire but even that isn't working now: thlorenz/proxyquire#236.

@codecov
Copy link

codecov bot commented Jan 30, 2019

Codecov Report

Merging #70 into master will decrease coverage by 68.6%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #70       +/-   ##
========================================
- Coverage    68.6%    0%   -68.61%     
========================================
  Files           7     2        -5     
  Lines         430    10      -420     
  Branches      120     3      -117     
========================================
- Hits          295     0      -295     
+ Misses         87    10       -77     
+ Partials       48     0       -48
Impacted Files Coverage Δ
src/index.ts 0% <0%> (-100%) ⬇️
src/debug.ts 0% <0%> (-40%) ⬇️
src/plugin.ts
src/config.ts
src/command.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5718233...3e32263. Read the comment docs.

@samuela
Copy link
Contributor Author

samuela commented Jan 30, 2019

@jdxcode Ok, fixed the failing test. Now codecov is complaining but I don't think I did anything wrong there...

Copy link
Contributor

@jdx jdx left a comment

Choose a reason for hiding this comment

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

looks good to me! thanks so much!

@jdx jdx merged commit 7324a99 into oclif:master Jan 30, 2019
@samuela samuela deleted the samuela/fix-tsconfig-parsing branch January 30, 2019 21:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants