Skip to content

fix: pass transformers object to ts.transpileModule method (Fixes #1051) - #1054

Merged
cspotcode merged 2 commits into
TypeStrong:masterfrom
thetutlage:master
May 25, 2020
Merged

fix: pass transformers object to ts.transpileModule method (Fixes #1051)#1054
cspotcode merged 2 commits into
TypeStrong:masterfrom
thetutlage:master

Conversation

@thetutlage

@thetutlagethetutlage commented May 23, 2020

Copy link
Copy Markdown
Contributor

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

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

codecovBot commented May 23, 2020

Copy link
Copy Markdown

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 
FlagCoverage Δ
#node_1072.26% <0.00%> (ø)
#node_12_1572.65% <0.00%> (ø)
#node_12_1672.65% <0.00%> (ø)
#node_1374.67% <0.00%> (ø)
#node_1474.67% <0.00%> (ø)
#typescript_2_774.34% <0.00%> (ø)
#typescript_latest73.51% <0.00%> (ø)
#typescript_next73.35% <0.00%> (ø)
#ubuntu74.50% <0.00%> (ø)
#windows74.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

coveralls commented May 23, 2020

Copy link
Copy Markdown

Coverage Status

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

@cspotcode

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

cspotcode commented May 24, 2020 via email

Copy link
Copy Markdown
Collaborator

@thetutlage

Copy link
Copy Markdown
ContributorAuthor

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

cspotcode commented May 24, 2020 via email

Copy link
Copy Markdown
Collaborator

@thetutlage

Copy link
Copy Markdown
ContributorAuthor

Does it look fine now?

@cspotcodecspotcode left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sweet, looks great!

@blakeembreyblakeembrey left a comment

Copy link
Copy Markdown
Member

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

cspotcode commented May 25, 2020 via email

Copy link
Copy Markdown
Collaborator

@cspotcode

Copy link
Copy Markdown
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:masterMay 25, 2020
@thetutlage

Copy link
Copy Markdown
ContributorAuthor

@cspotcode Any idea when this will be released?

@cspotcode

cspotcode commented Jun 2, 2020 via email

Copy link
Copy Markdown
Collaborator

Sign up for freeto 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.

4 participants

@thetutlage@coveralls@cspotcode@blakeembrey