Version 5 re-preparation #491

jamesrweb · 2025-01-30T19:16:19Z

Fixes #488

Proposed Changes

Use React 19.0.0 as the base version required
Expand error logging
Upgrade all packages
Update tooling configs and related settings
Formatting fixes
Linting fixes

Additional Notes (optional)

Work in progress but reviews and suggestions are, as always, welcome.

The demo app works perfectly with React 19 as-is. I don't know where the issue in #488 is coming from and I need proper direction to resolve and concrete and provable known issues that this library might be introducing but I am nearly 100% sure that we are not the root cause and it's something upstream either way. We don't do anything against the rules-of-react, otherwise we would have react compiler issues. Plus with the demo app and some of my own local projects that I tested with working fine locally with react 18 and 19, I am not sure what else we could do, please suggest areas for improvement for v5.x.x!

Lastly, I would like to simplify our implementation @yevdyko, such as a more checks and verifications in the automated deployment pipeline to avoid deployment issues, reductions in dependencies, less configurations, etc, where possible. Open to suggestions and other PRs on this also. Either way, I will leave this PR as it is until you get chance to review and test properly on your side and await your feedback / suggestions / contributions first @yevdyko.

…g and linting fixes

Qanpi · 2025-01-31T08:09:03Z

package.json

-    "react": ">= 18.2.0",
-    "react-dom": ">= 18.2.0"
+    "p5": ">= 1.11.3",
+    "react": ">= 19.0.0",


This will complain (albeit not critically) if the consumer doesn't have react >= 19.x.

Is that intended? Ideally, the library should probably support the earliest possible version of React.

Since this will be a major release, I'm thinking it's fine for 4.x.x users to be able to use react 18 and use the 5.x.x breaking change to update the new users or upgrading users to react 19. WDYT @yevdyko and @Qanpi, makes sense?

I believe the 5th version should be available for users on both React 18 and 19 to provide a lighter wrapper. This is the primary difference from version 4. Otherwise, it won't be accessible until the app is updated to React 19, which could be a blocker due to breaking changes.

Maybe but if we use 18 and 19 we could cause issues for the people on 19 since theres some deprecations in that version and also we use the react compiler in the vite setup now also. I don't know if we can reasonably guarantee full support for users of both 18 and 19, at least not 100% compatability can be guaranteed, thin or not. Does that make sense what I am trying to say?

That makes sense, but maybe we can add improvements that reduce package size to version 4 as well. WDYT?

@jamesrweb Taking into account that these changes work in both version 18 and 19 of React, maybe we can leave the requirements for React peer dependencies as >= 18.2.0. WDYT?

Qanpi · 2025-01-31T08:17:21Z

config/typescript/tsconfig.json

I would suggest you include "exclude": ["../../dist"] somewhere in this config.

Otherwise, vscode TS plugin complains because technically the dist folder is included by the "include": ["../../"] config setting. This is probably not critical, but will also cause typescript to build from the dist folder and may introduce unintended side effects.

Better yet, why not create another issue/PR to move this config and others to the root instead of keeping them in their own subdirectories?

Actually, on second thought it's better if you just change the include property to point to `["../../src", "../../demo"]. Otherwise, you're basically including the entire root directory into the Typescript config which makes little sense.

Yeah, that makes sense. I'll update the TS config and test it out 👌🏻

For the "root" topic, I'm not a fan of having config files in the root to be honest, I find it distracting to have a load of files right in the root which is why we initially agreed to move them to the config directory.

If we could cut down on the tooling though, I'd be open to it, for example: replacing eslint and prettier with standardjs or something but that has its own tradeoffs that we didn't explore yet.

Qanpi · 2025-01-31T08:27:52Z

I added some seeds for discussion, but ideally I'd like to see the react-19 fix out as soon as possible :)

jamesrweb · 2025-01-31T21:33:02Z

While working a little more on things today, I have noticed a weird issue with vite-plugin-dts. It seems to be unable to resolve dynamic import paths, however, I don't know why this would be and cannot see any other reports of this in the issues list of that repository. I wonder if either of you know why this may be or have an idea for a fix?

For the second issue in that file, the p5 import, that can be ignored, it was a tsconfig issue but I fixed that already. Still, the first import path is incorrect and I realise now that it has actually been an unnoticed issue for a while now in the 5.x.x rc chain. I guess this is the last hurdle before we can publish the next rc version.

As for the v18.x.x and v19.x.x of React discussion, I am open to keeping v18.x.x support but in that case I would ask @yevdyko to test an app using both versions under the version in this PR. I am curious if it can be published as-is or if we need to make changes and if we do, what and why. I would still push for making 19.x.x the default in v5.x.x because if we want to make 19.x.x the default only in future then we would have to release v6.x.x at that point. Still, I am open to it but please test things locally @yevdyko on existing projects with both versions and verify that this version is good to be deployed, otherwise send change suggestions or make PR targeting this branch and I will update / merge them ASAP.

yevdyko · 2025-02-01T13:36:42Z

@jamesrweb Sounds good, I'll try to test this branch on React 18 and 19 over the weekend and leave feedback after that. Thanks for your efforts!

Qanpi · 2025-02-03T08:14:40Z

I have noticed a weird issue with vite-plugin-dts

Yeah, not sure what's going on there. I tried changing the ReactP5WrapperGuard to a const arrow-function and then exporting as export default ReactP5WrapperGuard. That fixed the issue with the component's own type, but introduced two others for resolving the types of SketchProps and P5WrapperProps...

Qanpi

Besides the vite-plugin-dts issue (which can also be solved separately), this looks good to me.

I still think the library should support react>=18 if possible just to allow flexibility, assuming this causes no conflicts.

yevdyko · 2025-02-04T16:33:43Z

@jamesrweb @Qanpi I tried testing a branch with fixes inside a React app, but manually building the package inside node modules doesn't work? Have any of you tried this in your own applications?

See the error below:

> tsc --noEmit && vite build --config config/vite/vite.component.config.ts

src/components/ReactP5WrapperGuard.tsx:20:25 - error TS2742: The inferred type of 'ReactP5WrapperGuard' cannot be named without a reference to '@p5-wrapper/react/node_modules/@types/react/jsx-runtime'. This is likely not portable. A type annotation is necessary.

20 export default function ReactP5WrapperGuard<Props extends SketchProps>(
                           ~~~~~~~~~~~~~~~~~~~

src/components/ReactP5WrapperWithSketch.tsx:12:25 - error TS2742: The inferred type of 'ReactP5WrapperWithSketch' cannot be named without a reference to '@p5-wrapper/react/node_modules/@types/react/jsx-runtime'. This is likely not portable. A type annotation is necessary.

12 export default function ReactP5WrapperWithSketch<Props extends SketchProps>(
                           ~~~~~~~~~~~~~~~~~~~~~~~~


Found 2 errors in 2 files.

Errors  Files
     1  src/components/ReactP5WrapperGuard.tsx:20
     1  src/components/ReactP5WrapperWithSketch.tsx:12
 ELIFECYCLE  Command failed with exit code 2.
 ELIFECYCLE  Command failed with exit code 2.

jamesrweb · 2025-02-04T17:48:32Z

How did you get this?

Locally when I build everything is fine. Did you install all dependencies before running the build?

What's your typescript version (if global)?

Never seen this on the local build, local demo run or local project test... 😅

@yevdyko I'm very confused how you achieved this... 🤣

yevdyko · 2025-02-04T18:18:51Z

@jamesrweb Sure, this is what I've done:

Installed the package from a branch on the Github repo:

  "dependencies": {
    "@p5-wrapper/react": "github:P5-wrapper/react#v5.x.x-fixes",
    ...
  }

Then installed the package dependencies and tried to build it:

cd node_modules/@p5-wrapper/react && pnpm install && pnpm build

After that, it crashed with the error above.

The package itself compiled without problems, as you said, but not as part of the React application. In my case it's a React app based on JS, so the version of Typescript is the same as the one specified in the package.

Qanpi · 2025-02-05T09:33:12Z

I added #494 which fixes the type resolution issue @jamesrweb brought up above in relation to ReactP5WrapperGuard. It's a bit of a strange fix, but I've tested it on my demo apps, and it now resolves the types without issues in the build files. I'd appreciate it if @yevdyko or @jamesrweb could test this out too.

As for what @yevdyko mentioned later, I was able to get VSCode to highlight me the error when browsing node_modules/@p5-wrapper/react/src. I've encountered a similar issue in other projects before, and it's basically just complaining that jsxRuntime should be externalised.

The strange part is that the build breaks in your case @yevdyko. In my case, the build still passes even though VSCode complains with a red underline. In general, this should not be a problem because jsxRuntime is externalised in vite.component.config.ts. I guess it's some kind of conflict in vite/typescript configuration that causes this for you @yevdyko. Is your vite config resolving properly?

Qanpi · 2025-02-05T09:51:59Z

Actually, removing "composite": true from tsconfig.json according to this answer resolved the VSCode error highlights for me.

I included this in a separate commit in #494. Could you try this out following the steps you mentioned @yevdyko?

#494)

jamesrweb · 2025-02-05T11:42:20Z

I merged #494 into this branch now, you can test again @yevdyko for your case and I'll test the bundling in this PR again also for the type definitions. Thank you @Qanpi 🔥❤️

yevdyko · 2025-02-05T12:01:24Z

@jamesrweb I tested the changes to tsconfig suggested by @Qanpi and then tried running the fixes on apps with React 18 and 19 using the patched branch. Everything works as expected.

@Qanpi On the side note, I think your assumption why in my case the build was crashing and yours was not is close to reality, because my React app is built with vite as well, and the configuration may not be compatible with the configuration used in the package.

Thank you @Qanpi for the contribution and @jamesrweb for making it happen. Great job! 🥇 I think it's ready to go 🎸

yevdyko

🕺

Expand error logging, upgrade all packages, update configs, formattin…

454249f

…g and linting fixes

jamesrweb added enhancement help wanted dependencies Pull requests that update a dependency file security This label applies to security issues documentation Pull requests that update project documentation labels Jan 30, 2025

jamesrweb requested a review from yevdyko January 30, 2025 19:16

jamesrweb self-assigned this Jan 30, 2025

This was referenced Jan 30, 2025

bump vitest & vite react plugin (#488) #490

Closed

Font.textBounds() throws "J is not defined" #465

Open

jamesrweb changed the title ~~Expand error logging, upgrade all packages, update configs, formattin…~~ Version 5 re-preparation Jan 30, 2025

Qanpi reviewed Jan 31, 2025

View reviewed changes

Config path changes and build testing

c79d830

jamesrweb requested a review from Qanpi January 31, 2025 21:35

Qanpi approved these changes Feb 3, 2025

View reviewed changes

change typescript module resolution to resolve typescript resolution … (

d741337

#494)

jamesrweb linked an issue Feb 5, 2025 that may be closed by this pull request

type ReactP5WrapperGuard isn't properly resolved when building #493

Open

yevdyko approved these changes Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version 5 re-preparation #491

Version 5 re-preparation #491

jamesrweb commented Jan 30, 2025 •

edited

Loading

Qanpi Jan 31, 2025

jamesrweb Jan 31, 2025

yevdyko Jan 31, 2025

jamesrweb Jan 31, 2025

yevdyko Jan 31, 2025

yevdyko Feb 5, 2025 •

edited

Loading

Qanpi Jan 31, 2025 •

edited

Loading

Qanpi Jan 31, 2025

jamesrweb Jan 31, 2025

Qanpi commented Jan 31, 2025

jamesrweb commented Jan 31, 2025

yevdyko commented Feb 1, 2025

Qanpi commented Feb 3, 2025

Qanpi left a comment •

edited

Loading

yevdyko commented Feb 4, 2025 •

edited

Loading

jamesrweb commented Feb 4, 2025 •

edited

Loading

yevdyko commented Feb 4, 2025

Qanpi commented Feb 5, 2025 •

edited

Loading

Qanpi commented Feb 5, 2025

jamesrweb commented Feb 5, 2025

yevdyko commented Feb 5, 2025 •

edited

Loading

yevdyko left a comment

Version 5 re-preparation #491

Are you sure you want to change the base?

Version 5 re-preparation #491

Conversation

jamesrweb commented Jan 30, 2025 • edited Loading

Proposed Changes

Additional Notes (optional)

Qanpi Jan 31, 2025

Choose a reason for hiding this comment

jamesrweb Jan 31, 2025

Choose a reason for hiding this comment

yevdyko Jan 31, 2025

Choose a reason for hiding this comment

jamesrweb Jan 31, 2025

Choose a reason for hiding this comment

yevdyko Jan 31, 2025

Choose a reason for hiding this comment

yevdyko Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Qanpi Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Qanpi Jan 31, 2025

Choose a reason for hiding this comment

jamesrweb Jan 31, 2025

Choose a reason for hiding this comment

Qanpi commented Jan 31, 2025

jamesrweb commented Jan 31, 2025

yevdyko commented Feb 1, 2025

Qanpi commented Feb 3, 2025

Qanpi left a comment • edited Loading

Choose a reason for hiding this comment

yevdyko commented Feb 4, 2025 • edited Loading

jamesrweb commented Feb 4, 2025 • edited Loading

yevdyko commented Feb 4, 2025

Qanpi commented Feb 5, 2025 • edited Loading

Qanpi commented Feb 5, 2025

jamesrweb commented Feb 5, 2025

yevdyko commented Feb 5, 2025 • edited Loading

yevdyko left a comment

Choose a reason for hiding this comment

jamesrweb commented Jan 30, 2025 •

edited

Loading

yevdyko Feb 5, 2025 •

edited

Loading

Qanpi Jan 31, 2025 •

edited

Loading

Qanpi left a comment •

edited

Loading

yevdyko commented Feb 4, 2025 •

edited

Loading

jamesrweb commented Feb 4, 2025 •

edited

Loading

Qanpi commented Feb 5, 2025 •

edited

Loading

yevdyko commented Feb 5, 2025 •

edited

Loading