WIP: tidy up dependencies #418

Zyie · 2023-02-20T10:10:08Z

No description provided.

codesandbox-ci · 2023-03-02T15:07:02Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c3a34e6:

Sandbox	Source
sandbox	Configuration

Zyie · 2023-03-02T15:11:39Z

packages/react-fiber-react-18/src/index.js

-export const REACT_DOM_VERSION = rootPackageJson.dependencies[
+export const REACT_DOM_VERSION = rootPackageJson?.dependencies?.[
    'react-dom'
-].replace(/[^0-9.]/g, '');
+]?.replace(/[^0-9.]/g, '') || '18.0.0';


I dont think this code will work outside of the monorepo as rootPackageJson may not exist?

Seems like this could cause issues. Is there another way of getting this information?

Could we just expose a injectIntoDevTools function that the user has to call manually?

Not directly related to this line, but to the whole file.

I am pretty sure version passed to injectIntoDevTools doesn't need to be the version of ReactDOM any more. See:
1.1. Advertise a React-equivalent version number to the React DevTools pmndrs/react-three-fiber#1231
1.2. Reconciler should inject its own version into DevTools hook facebook/react#21268
1.3. DevTools should use reconciler version (rather than renderer version) facebook/react#21269

Please check if the package.json file imports are removed from the bundle. From my experience with rollup when I encountered this issue they were not. This increased bundle size unnecessarily. We can provide values read from package json file with the replace rollup plugin similar to __DEV__ value.

@Zyie does the code need to work outside of a monorepo? There's already quite a few things that depend on it being inside a monorepo - eslint, jest config and rollup config to name a few. I agree this code is nasty though. We could install React inside just this package, but because almost all the packages depend on it (as with other shared dependencies) I've listed them in the root package.json to keep versioning easier to manage.

@michalochman

From your links It appears that the version needs to be the current "React version" whether from DOM or React.

I can confirm that right now the whole package.json file was being bundled into the build. But in my Typescript PR, it wouldn't let me do an import from outside the package anyway, so I've already written a script to handle this situation and write a json file inside the src dir: https://github.com/pixijs/pixi-react/pull/412/files#diff-68eaf1ddcab994998bf4397fbcd8f7685590ac3760239880627679d5141c3ff4

Zyie · 2023-03-02T15:13:50Z

packages/docs/src/theme/ReactLiveScope/index.js

@@ -2,7 +2,7 @@ import React from 'react';
 import * as PIXI from 'pixi.js';
 import * as ReactPixi from '@pixi/react';
 import * as ReactPixiAnimated from '@pixi/react-animated';
-import { Spring } from 'react-spring';
+import { Spring } from '@react-spring/web';


For the docs i also used react-spring/web. The internet said this was better?

Ah for sure! That would also likely remove some npm install errors as some of the other react spring packages have old peer deps. We should likely do this everywhere!

baseten

Sorry lots of questions here! But I'm curious about why you've made some of the changes? 😄

baseten · 2023-03-02T18:26:23Z

packages/animated/package.json

+  "dependencies": {
+    "@pixi/react": "*"
+  },


I believe this is one case that should be a peer dependency shouldn't it? It adds additional functionality on top of pixi-react

Oh I thought this package was meant to be used instead of pixi-react

So you would only install @pixi/react-animated not both?
Maybe I misunderstood

No the intention is you install both of them together. @pixi/react-animated is a very small wrapper around the normal lib.

packages/react-components-pixi-7/package.json

baseten · 2023-03-02T18:30:54Z

packages/react-components-pixi-7/package.json

    "@testing-library/react": "^13.4.0",
-    "canvas": "^2.11.0",


Isn't this needed for tests?

I dont see it being used anywhere?
I just see jest-webgl-canvas-mock being used

I'll take a look, it was already in the original codebase

It's actually a peer dependency of js-dom: https://github.com/jsdom/jsdom/blob/master/package.json#L51 but the tests seem to run OK without it 🤔

baseten · 2023-03-02T18:32:55Z

package.json

-  "dependencies": {
-    "@babel/runtime": "^7.14.8",
-    "@pixi/app": "^7.1.1",
-    "@pixi/constants": "^7.1.1",
-    "@pixi/core": "^7.1.1",
-    "@pixi/display": "^7.1.1",
-    "@pixi/extensions": "^7.1.1",
-    "@pixi/graphics": "^7.1.1",
-    "@pixi/math": "^7.1.1",
-    "@pixi/mesh": "^7.1.1",
-    "@pixi/mesh-extras": "^7.1.1",
-    "@pixi/particle-container": "^7.1.1",
-    "@pixi/sprite": "^7.1.1",
-    "@pixi/sprite-animated": "^7.1.1",
-    "@pixi/sprite-tiling": "^7.1.1",
-    "@pixi/text": "^7.1.1",
-    "@pixi/text-bitmap": "^7.1.1",
-    "@pixi/ticker": "^7.1.1",
-    "@react-spring/animated": "^9.6.1",
-    "@react-spring/types": "^9.6.1",
-    "react": "^18.0.0",
-    "react-dom": "^18.0.0",
-    "react-spring": "^9.6.1"
-  },


While I don't entirely love either way of doing it, I'd deliberately added these shared packages to the root so it was easier to keep track of versioning across the whole monorepo, rather than specifying potentially separate versions in each package

My reasoning for this was that if we have a v6 component package and eventually a v8 package we would need to have individual packages defining there pixi versions?

Yes, good point 👍

baseten · 2023-03-02T18:33:53Z

packages/react-components-pixi-7/package.json

    "jest-webgl-canvas-mock": "^0.2.3",
-    "prop-types": "^15.8.1",


This is used in the Stage component isn't it?

Yeah it is, that is why it was moved to a dependency?

Sorry, missed that 👍

baseten · 2023-03-02T18:35:16Z

packages/react-fiber-react-18/package.json

  "devDependencies": {
+    "react-reconciler": "~0.29.0"
+  },
+  "peerDependencies": {
    "react-reconciler": "^0.29.0"
  },


So the react-reconciler is actually currently bundled into the library code (and always has been AFAIK) - so I'm not sure we need it as a peer dep do we?

Oh is the rollup config bundling devDependencies and not setting them as external?

I'm actually managing external libraries manually at the moment, perhaps I should move to your script but still override it in this case. I'll double check how this was done before and how react-three-fiber does it

react-three-fiber actually uses preconstruct which looks interesting. Anyway, they don't bundle it in, they just list it as a normal dependency so we can do the same

baseten · 2023-03-02T18:37:31Z

packages/react/package.json

-  "devDependencies": {
+  "dependencies": {
    "@pixi/react-components": "*",
    "@pixi/react-fiber": "*",
    "@pixi/react-invariant": "*",
-    "@pixi/react-modular": "*",
+    "@pixi/react-modular": "*"


These are bundled into the @pixi/react dist as a single package so we don't need them as dependencies do we?

I think I have misunderstood again how rollup is working in this project

Do we need to bundle everything into this package?

I feel like this should work more like pixi where the main pixi.js package is just a wrapper for a whole bunch of dependencies

Perhaps not, we could try it without bundling here, I guess I was just trying to emulate how the original package worked in spitting out a single build file and anyone that needed to configure things separately still could by importing the individual modules.

baseten · 2023-03-02T18:39:58Z

packages/react/package.json

-    "@pixi/math": ">=6.0.0",
-    "@pixi/app": ">=6.0.0",
-    "@pixi/ticker": ">=6.0.0",
-    "@pixi/display": ">=6.0.0",
-    "@pixi/core": ">=6.0.0",
-    "@pixi/sprite-animated": ">=6.0.0",
-    "@pixi/text-bitmap": ">=6.0.0",
-    "@pixi/graphics": ">=6.0.0",
-    "@pixi/mesh-extras": ">=6.0.0",
-    "@pixi/particle-container": ">=6.0.0",
-    "@pixi/constants": ">=6.0.0",
-    "@pixi/sprite": ">=6.0.0",
-    "@pixi/text": ">=6.0.0",
-    "@pixi/sprite-tiling": ">=6.0.0",
-    "prop-types": ">=15.0.0",


I wasn't 100% about this, but I added these here because I believe that peerDependencies, unlike other deps, aren't transitive? That is npm won't install the peerDependencies of a project's dependency or devDependency. Seeing as the intention here is that most users will only install and consume the single @pixi/react package, I thought we'd want to make sure that these were also installed for them?

I could be wrong about this but i believe the issue you ran into was that @pixi/react didn't define @pixi/react-components as a dependency because its getting bundled here

This meant that when you installed @pixi/react it didn't install @pixi/react-components and therefore didn't add the pixi peer dependencies

I believe that's not the case: https://dev.to/arcanis/implicit-transitive-peer-dependencies-ed0

It's not the end of the world to add them here too I don't think, it just means we need to be careful about making sure they match with the latest version of react-components-pixi-vX

I just had a thought, is the use of a single package (and package.json) with versioned subfolders going to cause us a problem here for react-fiber and react-components?

baseten · 2023-03-02T18:41:13Z

packages/animated/package.json

-  },
-  "gitHead": "6d5b98bc6d7eafd1b0016e60a725b2b6c9f46a33"
+  }


These get added automatically by lerna during publish: lerna/lerna#1880

oh strange this didn't happen for pixi or assetpack 🙁

Alex Prokop and others added 10 commits February 14, 2023 17:09

rollup config tweaks

bd515c4

package tweaks

02e0b28

clean up animated package

74cbabf

split out invariant util into its own package

9a14576

split out react-components package

d5d988e

split out react-reconciler into react-fiber package

1bfdf02

add react-modular package to wire everything together

405210e

update main pixi-react package to be a wrapper for other modules

bdeeafb

codesandbox ci tweaks

504fe15

WIP: tidy up dependencies

c672de0

Zyie force-pushed the module-package-tidy branch from 08bd75d to e901c8a Compare March 2, 2023 14:35

update deps

c3a34e6

Zyie force-pushed the module-package-tidy branch from e901c8a to c3a34e6 Compare March 2, 2023 15:04

Zyie commented Mar 2, 2023

View reviewed changes

baseten reviewed Mar 2, 2023

View reviewed changes

Base automatically changed from modular-packages to master March 13, 2023 12:57

Zyie closed this Apr 28, 2023

trezy deleted the module-package-tidy branch July 26, 2024 17:27

Uh oh!

WIP: tidy up dependencies #418

WIP: tidy up dependencies #418

Uh oh!

Conversation

Zyie commented Feb 20, 2023

Uh oh!

codesandbox-ci bot commented Mar 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baseten Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baseten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baseten Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zyie Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baseten Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baseten Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baseten Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

baseten Mar 2, 2023 •

edited

Loading

baseten Mar 3, 2023 •

edited

Loading

Zyie Mar 2, 2023 •

edited

Loading

baseten Mar 3, 2023 •

edited

Loading

baseten Mar 3, 2023 •

edited

Loading

baseten Mar 2, 2023 •

edited

Loading

baseten Mar 3, 2023 •

edited

Loading