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

fix: tsconfig.json should be respected by ts-node #44

Closed
wants to merge 2 commits into from

Conversation

jstewmon
Copy link

I spent a few hours today trying to figure out why the code generated by tsc did not match code being generated when I ran my commands. Eventually, I figured out that my tsconfig.json file was ignored when running the command. This was quite frustrating.

I suspect there may have been a historical reason for ignoring the tsconfig.json, like before d120ec1, it may not have been found correctly, but everything seems to work as expected for my project by letting ts-node load the project config.

@salesforce-cla
Copy link

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

@codecov
Copy link

codecov bot commented Aug 16, 2018

Codecov Report

Merging #44 into master will decrease coverage by 0.87%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
- Coverage   62.47%   61.59%   -0.88%     
==========================================
  Files           7        7              
  Lines         421      414       -7     
  Branches      115      113       -2     
==========================================
- Hits          263      255       -8     
  Misses        113      113              
- Partials       45       46       +1
Impacted Files Coverage Δ
src/ts_node.ts 76.19% <100%> (+0.68%) ⬆️
src/config.ts 56.02% <0%> (-1.58%) ⬇️

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 b822437...4e69aa9. Read the comment docs.

@jdx
Copy link
Contributor

jdx commented Aug 16, 2018

This won't work in situations where there are multiple tsconfig files.

@jstewmon
Copy link
Author

@jdxcode can you elaborate? I'm using a monorepo layout with a tsconfig file in each package, which extends a parent tsconfig file.

@jdx
Copy link
Contributor

jdx commented Aug 16, 2018

If you linked a plugin that had a different tsconfig then this would just use the first one. Better to just be consistent and not allow that to happen, that's why I took this functionality out.

@jdx
Copy link
Contributor

jdx commented Aug 16, 2018

Even in a monorepo setup you could have different tsconfig

@jstewmon
Copy link
Author

Ok, I understand your concern.

I don’t agree that consistently ignoring the tsconig is better, though. The setting may affect whether or not the transpiled code works - in my case, I’ve opted in to the new esModuleInterop so the oclif setting result in runtime errors.

I have a few ideas maybe you can give some feedback on...

  1. oclif option to disable ts-node and require the user to run tsc.
  2. oclif option to respect tsconfig.json with caveat that first or last config wins (I’m not actually clear which one it is). You said first one, but doesn’t the registration code get run for each plugin? Did I overlook a cache?
  3. I can submit a PR to ts-node to scope the config by path. It looks like it should be fairly easy.

// written on mobile. Please forgive typos or nonsense :)

@jdx
Copy link
Contributor

jdx commented Aug 16, 2018

While runtime errors aren't ideal, it's still not as bad as the confusing behavior that could result from overriding configuration.

If it's possible to fix ts-node to use different tsconfig's per path that is probably the most ideal solution. Otherwise, we could just add esModuleInterop if any of the tsconfig's set it.

@jdx
Copy link
Contributor

jdx commented Aug 16, 2018

I feel typescript 3.x project references might be relevant here as well but I haven't investigated much (and it's likely not yet used by ts-node anyways)

@jstewmon
Copy link
Author

I’m actually using project references but I’m new to typescript, so I’m not sure the relevance. I thought they’re just used for type resolution, not transpilation (compiler settings don’t necessarily apply).

I’ll look into it more tomorrow.

It’s not clear to me that I’ve done anything especially rare, so I’m still not understanding your perspective on how the current implementation is not the most confusing for a developer. I expected my tsconfig.json to be used, and it took me a few hours to figure out why it wasn’t being used.

I think we’ll work out a good solution but if it takes a while, would you be interested in updating the docs in the meantime to emphasize how ts-node is used to facilitate plugin development?

@jdx
Copy link
Contributor

jdx commented Aug 17, 2018

This change would make it so adding a new plugin could screw up an unrelated plugin. That's far more confusing of a situation than the current which just doesn't respect tsconfig at all.

@jstewmon
Copy link
Author

@jdxcode I submitted TypeStrong/ts-node/pull/668 to ensure the correct compiler settings are used with each registered typescript root.

I optimistically pruned a few more settings in this PR, which will be obviated if the ts-node PR is accepted.

@jstewmon
Copy link
Author

Also, I'm not sure why, but the disk cache seems to have started working with these changes - my commands previously had 2-3 seconds of latency on every run, but now the delay is virtually imperceptible.

src/ts_node.ts Outdated
rootDirs,
typeRoots,
}
typeCheck: true
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't have to be specified as it's the default now in ts-node I think

@salesforce-cla
Copy link

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Jonathan Stewmon <j***@r***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated.

@mshanemc
Copy link
Member

this repo is not accepting PRs except for security. Please use https://github.com/oclif/core

@mshanemc mshanemc closed this May 15, 2023
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.

3 participants