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: pass transformers object to ts.transpileModule method (Fixes #1051) #1054

Merged
merged 2 commits into from May 25, 2020

Conversation

thetutlage
Copy link
Contributor

@thetutlage thetutlage commented May 23, 2020

As per the README, only the transformers factory function is not allowed during the transpileOnly module. However, an object can still be passed.

In this PR https://github.com/TypeStrong/ts-node/pull/879/files#diff-f41e9d04a45c83f3b6f6e630f10117feR383, the code already guards against the factory function, but doesn't pass the transformers to the transpileModule method.

Related issue #1051

@thetutlage thetutlage changed the title fix: pass transformers object to ts.transpileModule method fix: pass transformers object to ts.transpileModule method (Fixes #1051) May 23, 2020
@codecov
Copy link

codecov bot commented May 23, 2020

Codecov Report

Merging #1054 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1054   +/-   ##
=======================================
  Coverage   74.83%   74.83%           
=======================================
  Files           6        6           
  Lines         608      608           
  Branches      142      142           
=======================================
  Hits          455      455           
  Misses        100      100           
  Partials       53       53           
Flag Coverage Δ
#node_10 72.26% <0.00%> (ø)
#node_12_15 72.65% <0.00%> (ø)
#node_12_16 72.65% <0.00%> (ø)
#node_13 74.67% <0.00%> (ø)
#node_14 74.67% <0.00%> (ø)
#typescript_2_7 74.34% <0.00%> (ø)
#typescript_latest 73.51% <0.00%> (ø)
#typescript_next 73.35% <0.00%> (ø)
#ubuntu 74.50% <0.00%> (ø)
#windows 74.67% <0.00%> (ø)

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 e567002...a6aa25b. Read the comment docs.

@coveralls
Copy link

coveralls commented May 23, 2020

Coverage Status

Coverage increased (+1.03%) to 83.019% when pulling a6aa25b on thetutlage:master into e567002 on TypeStrong:master.

@cspotcode
Copy link
Collaborator

@thetutlage Thanks for the speedy PR!

What is the reason for moving the assertion into getOutputTranspileOnly? If I'm reading the change correctly, this change delays the TypeError so it's no longer thrown by register() or create() but is instead thrown by require() calls.

@thetutlage
Copy link
Contributor Author

The main reason for moving the typeof transformers === 'function' check inside the getOutputTranspileOnly method is to narrow down the transformers type.

Inside getOutputTranspileOnly the transformers variable is typed as follows

Screen Shot 2020-05-24 at 11 33 36 AM

Now, in order to narrow it down, we have two options

  • Move the if condition inside this method (like I did)
  • Manually type cast transformers to Exclude the factory function

What you think?

@cspotcode
Copy link
Collaborator

cspotcode commented May 24, 2020 via email

@thetutlage
Copy link
Contributor Author

If I get it right, you are saying to move the typeof check to its original place (where it was earlier) and instead go with 2nd option, ie: Manually type cast transformers to Exclude the factory function

Right?

@cspotcode
Copy link
Collaborator

cspotcode commented May 24, 2020 via email

@thetutlage
Copy link
Contributor Author

Does it look fine now?

Copy link
Collaborator

@cspotcode cspotcode left a comment

Choose a reason for hiding this comment

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

Sweet, looks great!

Copy link
Member

@blakeembrey blakeembrey left a comment

Choose a reason for hiding this comment

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

@cspotcode Is there any reason we shouldn't move the getOutputTranspileOnly function back within the else block?

@cspotcode
Copy link
Collaborator

cspotcode commented May 25, 2020 via email

@cspotcode
Copy link
Collaborator

Nevermind, I'm going to do it as a separate PR, cuz it'll be clearer that way.

@cspotcode cspotcode merged commit 1f5b1e0 into TypeStrong:master May 25, 2020
@thetutlage
Copy link
Contributor Author

@cspotcode Any idea when this will be released?

@cspotcode
Copy link
Collaborator

cspotcode commented Jun 2, 2020 via email

@cspotcode cspotcode mentioned this pull request Aug 21, 2020
4 tasks
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

4 participants