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

v2pre ts bug, not happy with typed const arrow functions #7295

Closed
1 task done
xHomu opened this issue Aug 29, 2023 · 28 comments · Fixed by #7306
Closed
1 task done

v2pre ts bug, not happy with typed const arrow functions #7295

xHomu opened this issue Aug 29, 2023 · 28 comments · Fixed by #7306
Assignees
Labels
external feat:typescript v2 Issues related to v2 apis

Comments

@xHomu
Copy link
Contributor

xHomu commented Aug 29, 2023

What version of Remix are you using?

2.0.0-pre.2

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

Upgrading to v2 release candidate from v1.19.3 is fairly painless on mana.wiki, except for this one weird Typescript error that I don't know what might be the sourced from:

image

$ remix dev -c "npm run dev:node" --manual

 💿  remix dev

 info  building...
X [ERROR] The character ">" is not valid inside a JSX element

    app/utils/swr-fetchers.ts:25:48:
      25 │ export const swrGqlFetcher = <T>(props: Props) => {
         ╵                                                 ^

  TypeScript's TSX syntax interprets arrow functions with a single generic type parameter as an opening JSX element. If you want it to be interpreted as an arrow function instead, you need to add a trailing comma after the type parameter to 
disambiguate:

    app/utils/swr-fetchers.ts:25:29:
      25 │ export const swrGqlFetcher = <T>(props: Props) => {
         │                              ~~~
         ╵                              <T,>


X [ERROR] Unexpected "const"

    app/utils/swr-fetchers.ts:26:3:
      26 │    const { query, variables } = props;
         ╵    ~~~~~


 info  rebuilding... (~ app\utils\swr-fetchers.ts)
 info  building...
X [ERROR] The character ">" is not valid inside a JSX element

    app/utils/swr-fetchers.ts:26:48:
      26 │ export const swrGqlFetcher = <T>(props: Props) => {
         ╵                                                 ^

  TypeScript's TSX syntax interprets arrow functions with a single generic type parameter as an opening JSX element. If you want it to be interpreted as an arrow function instead, you need to add a trailing comma after the type parameter to 
disambiguate:

    app/utils/swr-fetchers.ts:26:29:
      26 │ export const swrGqlFetcher = <T>(props: Props) => {
         │                              ~~~
         ╵                              <T,>


X [ERROR] Unexpected "const"

    app/utils/swr-fetchers.ts:27:3:
      27 │    const { query, variables } = props;
         ╵    ~~~~~


 info  rebuilding... (~ app\utils\swr-fetchers.ts)

Updating the .ts files so they don't use arrow functions got the build working, otherwise, great release!

Expected Behavior

Remix shouldn't have an opinion about how I declare functions in typescript files.

Actual Behavior

Remix is very insistent on me stop using const arrow functions.

@AlemTuzlak
Copy link
Contributor

@xHomu I have the same issue, created a minimum repro here:
https://github.com/AlemTuzlak/remix-v2-ts-issue
You can reproduce this issue by cloning my repo, installing the deps and running npm run dev.
The issue seems to be with the remix.config.js,
If you add the following to ignoredRouteFiles:

const ignoredRoutePatterns = [
  "**/.*",
  "**/components/**",
  "**/integration/*.test.*",
];

module.exports = { 
  ignoredRouteFiles: ignoredRoutePatterns,
  serverDependenciesToBundle: [/^marked.*/],
  tailwind: true,
  serverModuleFormat: "cjs",
};

it breaks, but if you do the following:

const ignoredRoutePatterns = [
-  "**/.*",
+ "**/*",
  "**/components/**",
  "**/integration/*.test.*",
];

module.exports = { 
  ignoredRouteFiles: ignoredRoutePatterns,
  serverDependenciesToBundle: [/^marked.*/],
  tailwind: true,
  serverModuleFormat: "cjs",
};

it works

@brophdawg11 brophdawg11 added the v2 Issues related to v2 apis label Aug 30, 2023
@pcattori
Copy link
Contributor

pcattori commented Aug 30, 2023

@AlemTuzlak Setting **/* in ignoredRoutePatterns will make Remix ignore all of your route files, which only prevents the error by not processing any routes.

@xHomu Needing to disambiguate single-arg generics from JSX elements has been a thing in TS for a long time. Here's a playground repro: https://tsplay.dev/w6Vqyw . Note that changing the TS version to anything (3.3-5.2) results in the same error.

To fix your code, you don't have to remove arrow functions. You can put a comma after the T (or whatever type generic) as indicated in the error message: https://tsplay.dev/WPBJkN

Not sure why you'd only run into this in v2 and not 1.19.3, but maybe we fixed something in the compiler to be more TS compliant. But I'd argue that's a feature, not a bug.

@AlemTuzlak
Copy link
Contributor

AlemTuzlak commented Aug 30, 2023

@pcattori so what you're saying is we're idiots 🤣 (for anyone reading this in the future, it's a joke)

@xHomu
Copy link
Contributor Author

xHomu commented Aug 30, 2023

Well, he's not wrong!

@duailibe
Copy link

@pcattori isn't that the whole reason we have .ts and .tsx files, though? (to disambiguate JSX vs non-JSX files)

Looking at the screen @xHomu posted, the compiler is parsing a .ts as a JSX file, which is the real bug here.

@pcattori
Copy link
Contributor

@duailibe unfortunately, there's a ton of existing apps and libraries that use .js and .ts files for JSX and its not uncommon for other tools and frameworks to try to parse JSX in .js and .ts files. We've received issues in the past for not supporting JSX in .js and .ts files for this reason (e.g. #7051). So the decision was made to support JSX in .ts and .js files too.

@AlemTuzlak
Copy link
Contributor

.jsx I get but how can you have jsx in ts files without ts going crazy and complaining on you? I get why you made the decision but why would people ask for tsx in ts support when there is a clear destinction between the two

@duailibe
Copy link

duailibe commented Aug 30, 2023

@pcattori I understand the reasoning for .js files (since it's very unlikely that the parser will get confused by syntax - at least I'm not aware of any), but I strongly disagree with the decision for .ts files as it's fairly common practice to use the .tsx extension.

IMHO the best heuristics would be:

loader: path.endsWith(".js") || path.endsWith(".jsx") ? "jsx" : path.endsWith(".tsx") ? "tsx" : "ts"

or something similar (again, best in my opinion anyway).

@pcattori
Copy link
Contributor

To be clear, #7051 was a bug fix for a breaking change. The decision to support JSX in .js and .ts files was implicitly made long ago, before I was part of the Remix team. To drop support for JSX in .ts would constitute a breaking change for users on versions 1.19.0 or earlier.

@duailibe
Copy link

duailibe commented Aug 30, 2023

@pcattori Sorry for insisting but that's not why I see in my 1.19.3 app. Just rename a .tsx file to .ts:

❯ npx remix build
 info  building... (NODE_ENV=production)
✘ [ERROR] Expected ">" but found "className"

    app/components/location-selector.ts:22:9:
      22 │     <div className="flex bg-[#e7b348] h-8 items-center justify-cen...
         │          ~~~~~~~~~
         ╵          >

@AlemTuzlak
Copy link
Contributor

@pcattori Sorry for insisting but that's not why I see in my 1.19.3 app. Just rename a .tsx file to .ts:

❯ npx remix build
 info  building... (NODE_ENV=production)
✘ [ERROR] Expected ">" but found "className"

    app/components/location-selector.ts:22:9:
      22 │     <div className="flex bg-[#e7b348] h-8 items-center justify-cen...
         │          ~~~~~~~~~
         ╵          >

This would be the behaviour I would expect if there was JSX in a TS file, didn't even know theres a way to go around it

@pcattori
Copy link
Contributor

Ah ok then maybe I misspoke earlier. I assumed we had supported JSX in .ts before. Since that's not the case, I agree that we shouldn't parse .ts files with the tsx loader.

@pcattori pcattori reopened this Aug 30, 2023
@duailibe
Copy link

@pcattori thanks for the patience!

I believe that would align with how the loaders are configured for the build:

".js": "jsx",
".jsx": "jsx",

".ts": "ts",
".tsx": "tsx",

@pcattori pcattori linked a pull request Aug 30, 2023 that will close this issue
@pcattori pcattori added this to v2 Aug 30, 2023
@pcattori pcattori moved this to PR in v2 Aug 30, 2023
@pcattori pcattori moved this from PR to Merged in v2 Aug 30, 2023
@pcattori
Copy link
Contributor

Fixed by #7306

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.0-pre.4 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

xHomu added a commit to manawiki/mana that referenced this issue Aug 31, 2023
@xHomu
Copy link
Contributor Author

xHomu commented Aug 31, 2023

Can confirm 2.0.0-pre.4 resolved this
image

xHomu added a commit to manawiki/mana that referenced this issue Aug 31, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-6c0310c-20230901 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

🤖 Hello there,

We just published version 2.0.0-pre.6 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-9288516-20230902 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

🤖 Hello there,

We just published version 2.0.0-pre.7 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

🤖 Hello there,

We just published version 2.0.0-pre.7 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-481f73e-20230906 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

🤖 Hello there,

We just published version 2.0.0-pre.8 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-1a57073-20230907 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

🤖 Hello there,

We just published version 2.0.0-pre.9 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-1fac238-20230908 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.0-pre.10 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-3646f91-20230914 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external feat:typescript v2 Issues related to v2 apis
Projects
No open projects
Status: Merged
Development

Successfully merging a pull request may close this issue.

6 participants