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

The removal of TypeScript types may result in broken JavaScript code #56597

Closed
BlueNebulaDev opened this issue Jan 14, 2025 · 10 comments · Fixed by #56785
Closed

The removal of TypeScript types may result in broken JavaScript code #56597

BlueNebulaDev opened this issue Jan 14, 2025 · 10 comments · Fixed by #56785
Labels
confirmed-bug Issues with confirmed bugs. strip-types Issues or PRs related to strip-types support

Comments

@BlueNebulaDev
Copy link

BlueNebulaDev commented Jan 14, 2025

Version

v23.2.0

Platform

Linux 6.12.9 Fri Dec 20 10:11:49 UTC 2024 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Run the following TypeScript script with nodejs (with the --experimental-strip-types option, if necessary):

function mkId() {
	return <T>
		(x: T)=>x;
}

const id = mkId();
console.log(id(5));

How often does it reproduce? Is there a required condition?

It always happen deterministically.

What is the expected behavior? Why is that the expected behavior?

The script should print 5.

What do you see instead?

The script errors with:

console.log(id(5));
            ^

TypeError: id is not a function

Additional information

I believe that Node strips TS types in such a way that the script I showed gets turned into the following JS code:

function mkId() {
	return
		(x: T)=>x;
}

const id = mkId();
console.log(id(5));

And unfortunately Node interprets a return followed by a new line as a return;.

The same behavior happens with different (but similar code), for instance:

function mkId() {
	return <
		T
	>(x: T)=>x;
}

It is somewhat common to break templates on a new line, when the generic types are complex (for instance if they have long extend clauses) and this issue is particularly annoying because the IDE (which understands TS) doesn't flag it as an error.

@ljharb
Copy link
Member

ljharb commented Jan 14, 2025

This is due to ASI issues around return specifically; the proper fix is likely that swc should replace the snippets to the following, respectively:

function mkId() {
	return (
		(x   )=>x);
}


function mkId() {
	return (
		 
	 (x   )=>x);
}

@BlueNebulaDev
Copy link
Author

Yes, introducing parenthesis around the generic closure would do the trick.

@marco-ippolito marco-ippolito added the strip-types Issues or PRs related to strip-types support label Jan 14, 2025
@marco-ippolito
Copy link
Member

marco-ippolito commented Jan 14, 2025

We see how swc transpiles:

const { stripTypeScriptTypes } = require('node:module');

const code = stripTypeScriptTypes(`function mkId() {
  return <T>
    (x: T) => x;
}

const id = mkId();
console.log(id(5));`);

console.log(code);

Output:

function mkId() {
  return    
    (x   ) => x;
}

const id = mkId();
console.log(id(5));

I'm only afraid how we can fix this without changing locations

cc @kdy1
@robpalme I think you fixed this issue in ts-blank-space

@marco-ippolito marco-ippolito added the confirmed-bug Issues with confirmed bugs. label Jan 14, 2025
@ljharb
Copy link
Member

ljharb commented Jan 14, 2025

@marco-ippolito it can be fixed by using parens - basically it'd only change one location, where it adds the paren before the semicolon - otherwise it'd just be adding a paren instead of a blank space.

@marco-ippolito
Copy link
Member

marco-ippolito commented Jan 14, 2025

@marco-ippolito it can be fixed by using parens - basically it'd only change one location, where it adds the paren before the semicolon - otherwise it'd just be adding a paren instead of a blank space.

If we change even 1 location it would introduce edge cases.
Maybe with backticks we can overcome (hacky af)

return `
    `,(x ) => x;

@bakkot
Copy link
Contributor

bakkot commented Jan 14, 2025

Maybe with backticks we can overcome

More naturally (and works even if the second line isn't indented):

return 0,
(x ) => x;

@ljharb
Copy link
Member

ljharb commented Jan 14, 2025

I suppose the semicolon could be replaced with the paren. but yeah, 0, might work better.

@bakkot
Copy link
Contributor

bakkot commented Jan 14, 2025

I went ahead and opened an issue on SWC.

@robpalme
Copy link

Thanks for the report @BlueNebulaDev

Ashley and I made some bold claims about prizes for anyone who could break this. If you are ever in London and wish to collect, please let me know 😉

@BlueNebulaDev
Copy link
Author

Ohh, that sounds intriguing and fun!
I'm likely to be in London in late-spring/early-summer. I'll drop you a private message once my plans become more concrete.

nodejs-github-bot added a commit that referenced this issue Jan 29, 2025
PR-URL: #56785
Fixes: #56597
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. strip-types Issues or PRs related to strip-types support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants