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

Next 15 + React 19 #1233

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Next 15 + React 19 #1233

wants to merge 18 commits into from

Conversation

ahkhanjani
Copy link
Contributor

No description provided.

This reverts commit e588b04.
@jpainam
Copy link

jpainam commented Oct 27, 2024

Thanks for the migration.

i'm was able to pn build, pn lint and pn typecheck locally, but when i open localhost:3000, i get a lot of module not found errors

Module not found: Can't resolve @acme/ui/button

for every single import from packages.

Anything i'm missing out?

alias pn=pnpm

@ahkhanjani
Copy link
Contributor Author

i'm was able to pn build, pn lint and pn typecheck locally, but when i open localhost:3000, i get a lot of module not found errors

Should've been fixed. I'll look into it

@ahkhanjani
Copy link
Contributor Author

@jpainam Works fine for me. Make sure you've updated the exports field in packages/ui/package.json. Turpopack doesn't work with wildcard **/* exports.

@@ -10,14 +10,14 @@
"typecheck": "tsc --noEmit"
},
"dependencies": {
"@auth/core": "0.34.2"
"@auth/core": "0.37.2"
Copy link
Member

Choose a reason for hiding this comment

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

This upgrade messed up the expo auth last time I tried it. Didn't investigate further what it was though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look

apps/expo/tsconfig.json Outdated Show resolved Hide resolved
apps/nextjs/next.config.ts Outdated Show resolved Hide resolved
"prettier": "catalog:",
"turbo": "^2.1.3",
"turbo": "~2.1.3",
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turbo 2.2 crashes my projects in my work repo. I think it also did in this one. Not sure. I'll try again

Copy link
Contributor Author

@ahkhanjani ahkhanjani Oct 27, 2024

Choose a reason for hiding this comment

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

Yup. Also panics here. In every app dev server (nextjs/expo)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it happens with me as well. 2.2.3 is a bit better, but it still happens

@jpainam
Copy link

jpainam commented Oct 28, 2024

@jpainam Works fine for me. Make sure you've updated the exports field in packages/ui/package.json. Turpopack doesn't work with wildcard **/* exports.

That was the issue. Had to export each file individually

@juliusmarminge
Copy link
Member

@jpainam Works fine for me. Make sure you've updated the exports field in packages/ui/package.json. Turpopack doesn't work with wildcard **/* exports.

Is that a bug or intended behavior ?

@ahkhanjani
Copy link
Contributor Author

Is that a bug or intended behavior ?

I couldn't find anything mentioning this directly. There is this indirect mention. I think they simply don't support it.

@juliusmarminge
Copy link
Member

That's for turborepo though, I'm guessing the limitation you're pointing at is about turbopack?

Either way I'm fine with explicit exports just thought if we should file an issue to turbopack since afaik everything that works with webpack should work in turbopack too

@ahkhanjani
Copy link
Contributor Author

I'm guessing the limitation you're pointing at is about turbopack?

Yes

Either way I'm fine with explicit exports just thought if we should file an issue to turbopack since afaik everything that works with webpack should work in turbopack too

That's fair. I'm going to open an issue on vercel/next.js.

This aside though, it sucks to manually add the export for each ui element every time you install it but having autocomplete is worth it imo. Should we keep it explicit either way?

@juliusmarminge
Copy link
Member

This aside though, it sucks to manually add the export for each ui element every time you install it but having autocomplete is worth it imo. Should we keep it explicit either way?

I honestly thought we already were using explicit due to the limitation in auto-imports.

@ahkhanjani
Copy link
Contributor Author

I honestly thought we already were using explicit due to the limitation in auto-imports.

I made the commit that changed that due to us emitting type defs and the exports getting out of control. I'm dropping dev and build scripts inside ui package now for this reason.

@juliusmarminge
Copy link
Member

juliusmarminge commented Oct 28, 2024

I made the commit that changed that due to us emitting type defs and the exports getting out of control. I'm dropping dev and build scripts inside ui package now for this reason.

Hmm not sure I like that reasoning 😅 There's scripting that can be done to automate it if that's the main concern

But either way declaration emitting is to reduce the work the language server has to do and I'm not sure these React components are very heavy to infer... It's more for Zod, tRPC, Drizzle etc that does a lot of type shenanigans that you can notice the gains of declaration files vs live inference

@ahkhanjani
Copy link
Contributor Author

Either way I'm fine with explicit exports just thought if we should file an issue to turbopack since afaik everything that works with webpack should work in turbopack too

Turns out webpack is the problem:
evanw/esbuild#2974 (comment)
webpack/enhanced-resolve#400

I tried recreating the issue without using an array. This was what we were doing:

{
  "exports": {
    "./*": [
      "./src/*.ts",
      "./src/*.tsx"
    ]
  }
}

I just accidentally figured out that it works just fine without the array:

{
  "exports": {
    "./*": "./src/*.tsx"
  }
}

This is not a turbo issue. We could continue using the wildcard export for convenience and lose autocomplete (note that autocomplete works fine after importing an element manually the first time in a .ts file).

What should we do?

@ahkhanjani
Copy link
Contributor Author

Hmm not sure I like that reasoning 😅 There's scripting that can be done to automate it if that's the main concern

But either way declaration emitting is to reduce the work the language server has to do and I'm not sure these React components are very heavy to infer... It's more for Zod, tRPC, Drizzle etc that does a lot of type shenanigans that you can notice the gains of declaration files vs live inference

I agree. This PR fixes that. That was my bad 😅

We're keeping explicit exports then

@ahkhanjani
Copy link
Contributor Author

That was the issue. Had to export each file individually

@jpainam You can keep the wildcard export if it's more convenient for you. Just replace the array with the string as I explained and see if it works.

@juliusmarminge
Copy link
Member

The index file is a .ts so we need the array though (unless you change to index.tsx even for non-react stuff 😭)

@ahkhanjani
Copy link
Contributor Author

The index file is a .ts so we need the array though (unless you change to index.tsx even for non-react stuff 😭)

index.ts is exported separately. The reason we had the array was for specific files like use-toast.ts which gets added when installing toast (or so it did. haven't tried recently)

@ahkhanjani
Copy link
Contributor Author

I should've written the complete version of the exports field. My bad.

{
  "exports": {
    ".": "./src/index.ts",
    "./*": "./src/*.tsx"
  }
}

This should work IF you want to use the wildcard export. The recommendation is exporting each element separately. Also haven't figured out how to export wildcard .ts files yet. Probably add those manually

@juliusmarminge
Copy link
Member

Cool cool 👍👍

To make explicit exports less of a headache and risk of forgetting to add them, we could add a small script wrapping shadcn cli that after installation,

  • adds export paths in package.json
  • runs formatting

@ahkhanjani
Copy link
Contributor Author

Currently expo dev server is failing due to react versions clashing. Going to fix that

@ahkhanjani
Copy link
Contributor Author

To make explicit exports less of a headache and risk of forgetting to add them, we could add a small script wrapping shadcn cli that after installation,

  • adds export paths in package.json
  • runs formatting

Totally agree

@ahkhanjani
Copy link
Contributor Author

Encountered a potential pnpm bug while trying to fix expo
pnpm/pnpm#8715

@types/eslint__js is not needed anymore
@dBianchii
Copy link
Contributor

Shadcn will also update some of its components in general preparation for React 19:
https://ui.shadcn.com/docs/react-19

Not sure if they have updated it, just dropping the link here 👍

@ahkhanjani
Copy link
Contributor Author

Shadcn will also update some of its components in general preparation for React 19: https://ui.shadcn.com/docs/react-19

Not sure if they have updated it, just dropping the link here 👍

I couldn't find anything we can change in the page you mentioned. Seems like something for third-party cli packages. Am I missing something?

@juliusmarminge
Copy link
Member

Can remove all the forwardRef stuff but let's do that in a follow up PR

@ahkhanjani ahkhanjani mentioned this pull request Nov 9, 2024
@ahkhanjani
Copy link
Contributor Author

Shadcn will also update some of its components in general preparation for React 19: https://ui.shadcn.com/docs/react-19

Not sure if they have updated it, just dropping the link here 👍

Can remove all the forwardRef stuff but let's do that in a follow up PR

#1243

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

6 participants