-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(core): prevent breaking error when registering transpiler #16929
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
prevent 'fn is not a function' error when running registerTranspiler. depending on the version, @swc-node/register does not always return a cleanup function, so an explicit check for this is now added.
const cleanupFn = register(compilerOptions); | ||
const isFunction = typeof cleanupFn === 'function'; | ||
|
||
if (!isFunction) return () => {}; | ||
|
||
return cleanupFn; |
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.
Here is the fix.
The rest of the PR is purely refactor to avoid variable reassignment and reduce the amount of nesting. e.g. early returns.
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.
This makes sense, swapping to a ternary to avoid extra variables
type ISwcRegister = typeof import('@swc-node/register/register')['register']; | ||
type ISwcRegisterVoid = (...args: Parameters<ISwcRegister>) => void; | ||
|
||
// These are requires to prevent it from registering when it shouldn't | ||
const register = require('@swc-node/register/register').register as | ||
| ISwcRegister | ||
| ISwcRegisterVoid; |
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.
Add explicit type-checking to account for versions of @swc-node/register
that do not return cleanup functions.
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 removing some of this, since we are removing the strict portion it doesn't end up helping and looks a bit confusing
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.
Makes sense without strict null checks 👍
@@ -35,6 +35,59 @@ export const registerTsProject = ( | |||
}; | |||
}; | |||
|
|||
export function getSwcTranspiler( |
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.
Needed to enable strictNullChecks
otherwise TypeScript was allowing this function to return null
/undefined
even when the return type was a function.
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 hear you, and I agree, but I don't think we should do this via an extra tsconfig, since that won't get checked at build time.
We'd eventually like to enable strict mode for the nx package globally, but can't just yet. I've removed this part of the changes.
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.
Sounds good, and strict mode is a great idea for the entire project.
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.
Thanks for the fix @passbyval! I've made some changes and it looks good to go 🎉
@@ -35,6 +35,59 @@ export const registerTsProject = ( | |||
}; | |||
}; | |||
|
|||
export function getSwcTranspiler( |
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 hear you, and I agree, but I don't think we should do this via an extra tsconfig, since that won't get checked at build time.
We'd eventually like to enable strict mode for the nx package globally, but can't just yet. I've removed this part of the changes.
type ISwcRegister = typeof import('@swc-node/register/register')['register']; | ||
type ISwcRegisterVoid = (...args: Parameters<ISwcRegister>) => void; | ||
|
||
// These are requires to prevent it from registering when it shouldn't | ||
const register = require('@swc-node/register/register').register as | ||
| ISwcRegister | ||
| ISwcRegisterVoid; |
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 removing some of this, since we are removing the strict portion it doesn't end up helping and looks a bit confusing
const cleanupFn = register(compilerOptions); | ||
const isFunction = typeof cleanupFn === 'function'; | ||
|
||
if (!isFunction) return () => {}; | ||
|
||
return cleanupFn; |
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.
This makes sense, swapping to a ternary to avoid extra variables
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
Some versions of
@swc-node/register
do not return a cleanup function, causingregisterTsProject
to call anundefined
function:@swc-node/register
1.6.5 vs 1.4.2 for reference.Expected Behavior
To check
@swc-node/register
's return value, and return a no-op for versions that do not have a cleanup function.