-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Provide better error checking for transformed content #3807
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments inlined, looks solid.
return { | ||
process: jest.fn(), | ||
}; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we fit this in one line, or it's less readable?
() => ({process: jest.fn()})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier modified it to use this syntax :)
@@ -208,8 +208,13 @@ class ScriptTransformer { | |||
|
|||
if (typeof processed === 'string') { | |||
transformed.code = processed; | |||
} else { | |||
} else if (processed && typeof processed.code === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this code has potential to be run quite a lot of times (for every to-be-transformed file when there's no cache), we should be more explicit in terms of checking truthiness of processed
How about:
if (
processed !== null &&
typeof processed === 'object' &&
typeof processed.code === 'string'
) {
transformed = processed
}
It should produce a little more performant bytecode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll fail for undefined
. Unless we use != null
, but I haven't check if your eslint config allows eqeq null
.
Also typeof processed === 'object'
will pass for arrays as well, not only objects.
As anything other than null
or undefined
won't throw while using attribute accessor (eg. when processed is 42
or 'foo'
. I think we could just change it to:
processed != null && typeof processed.code === 'string'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also typeof processed === 'object'
will pass for arrays as well, not only objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As anything other than null
or undefined
won't throw while using attribute accessor (eg. when processed is 42
or 'foo'
. I think we could just change it to:
processed != null && typeof processed.code === 'string'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with that 👍
Codecov Report
@@ Coverage Diff @@
## master #3807 +/- ##
==========================================
+ Coverage 57.59% 57.64% +0.04%
==========================================
Files 194 194
Lines 6778 6780 +2
Branches 6 6
==========================================
+ Hits 3904 3908 +4
+ Misses 2871 2869 -2
Partials 3 3
Continue to review full report at Codecov.
|
Apparently your config permits |
Thank you! |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Let use know when
transform
function doesn't return required type of content.Resolves: #3779
Test plan
Added necessary tests to the suite itself.