-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Monorepo] Move dependencies to root package.json and change yarn to npm #18531
[Monorepo] Move dependencies to root package.json and change yarn to npm #18531
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is very concerning here is that individual packages use different versions of libraries that those one resolved in package-lock.json
:
- react
- react-native
In addition, can we avoid using ranges for packages in the root |
Hey @gziolo i hope it is not a problem that I pushed those changes here -#18566 |
Whatever works for you best. I don't care as long as at the end of the PR merging it's correct. Can you point me to the commit hash? I can't see the changes applied in #18566. If it helps I can also apply all changes myself on the top of the last PR you have. |
Sorry I haven't pushed the latest commit - 714f079 |
Cool, I left one comment. It looks great otherwise 👍 |
38bf6c6
to
6671a16
Compare
…d to fix all the errors later on
…ackages build for now
I think this is ready for another pass. I must admit there's a lot going on here. I kept the main idea of the PR @dratwas started: basically repair the dependencies and imports so that we can run again native tests. I have updated the description to give more details about all the changes, feel free to have a look. There is still a couple errors in the tests. One is a licence check error we could discuss here. @gziolo Do you think you'll have time to give this a look? If not could you ping someone from your team that might have time? Pinging some mobile devs who might be interested as well @hypest @maxme and @SergioEstevao who has helped on the RN upgrade. |
Seems those Apache 2.0 licensed dependencies aren't only for e2e tests (appium). |
@Tug I think we don't bundle About |
@@ -9,6 +9,7 @@ | |||
"noEmit": true, | |||
"resolveJsonModule": true, | |||
"target": "esnext", | |||
"types": ["react", "node", "jest", "requestidlecallback"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to limit the types used by tsc
, because I had conflicts in @types/react-native
such as
node_modules/@types/react-native/globals.d.ts (36,15): Duplicate identifier 'FormData'.
See this discussion DefinitelyTyped/DefinitelyTyped#15960
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use separate tsconfig files for the browser and native files... Separate them with include
and/or exclude
properties.
More specifically, I'd keep the main tsconfig.json as-is, and add in a tsconfig.native.json
for the native stuff and then just build the project in series.
A better option if you aren't set on the structure is to drop all react-native stuff somewhere with a common parent directory (e.g., packages/react-native/aztec/xxx
, packages/react-native/bridge/xxx
) and then just drop a tsconfig at packages/react-native/tsconfig.json
and let typescript do the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the main issue here is that tsc
loads everything installed in node_modules/@types
by default. And since we added react-native
as a dependency, new types are now installed with it that are causing conflicts in the main (web) build. So splitting the tsconfig
or adding a new one in our rn package won't address this problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which type definitions are conflicting? Sounds like they are defining ambients that are conflicting with node's ambients? There must be a situation where this has affected some other projects as well, no?
I'd be extremely cautious in specifying individual types by name because that could lead to a situation where, if someone were to install a new set of definitions without also including the name of those in the tsconfig, the bug would be difficult to pinpoint.
Additionally, I'm not sure how this would play with definitions that depend on other definitions. You would have to include those as well in your list of types. (i.e., this doesn't scale particularly well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions here @DanielRosenwasser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I have found this issue reported elsewhere online such as on DefinitelyTyped
(see first comment).
Here's the full report from running tsc
without this line in tsconfig.json
:
node_modules/@types/react-native/globals.d.ts:40:15 - error TS2300: Duplicate identifier 'FormData'.
40 declare class FormData {
~~~~~~~~
node_modules/typescript/lib/lib.dom.d.ts:5353:11
5353 interface FormData {
~~~~~~~~
'FormData' was also declared here.
node_modules/typescript/lib/lib.dom.d.ts:5363:13
5363 declare var FormData: {
~~~~~~~~
and here.
node_modules/@types/react-native/globals.d.ts:85:5 - error TS2717: Subsequent property declarations must have the same type. Property 'body' must be of type 'string | ArrayBuffer | ArrayBufferView | Blob | FormData | URLSearchParams | ReadableStream<Uint8Array> | null | undefined', but here has type 'string | ArrayBuffer | DataView | Int8Array | Uint8Array | Uint8ClampedArray | Int16Array | Uint16Array | Int32Array | Uint32Array | Float32Array | Float64Array | Blob | FormData | null | undefined'.
85 body?: BodyInit_;
~~~~
node_modules/typescript/lib/lib.dom.d.ts:1413:5
1413 body?: BodyInit | null;
~~~~
'body' was also declared here.
node_modules/@types/react-native/globals.d.ts:111:14 - error TS2300: Duplicate identifier 'RequestInfo'.
111 declare type RequestInfo = Request | string;
~~~~~~~~~~~
node_modules/typescript/lib/lib.dom.d.ts:18568:6
18568 type RequestInfo = Request | string;
~~~~~~~~~~~
'RequestInfo' was also declared here.
node_modules/@types/react-native/globals.d.ts:130:13 - error TS2403: Subsequent variable declarations must have the same type. Variable 'Response' must be of type '{ new (body?: string | ArrayBuffer | ArrayBufferView | Blob | FormData | URLSearchParams | ReadableStream<Uint8Array> | null | undefined, init?: ResponseInit | undefined): Response; prototype: Response; error(): Response; redirect(url: string, status?: number | undefined): Response; }', but here has type '{ new (body?: string | ArrayBuffer | DataView | Int8Array | Uint8Array | Uint8ClampedArray | Int16Array | Uint16Array | Int32Array | Uint32Array | Float32Array | Float64Array | Blob | FormData | null | undefined, init?: ResponseInit | undefined): Response; prototype: Response; error: () => Response; redirect: (url: ...'.
130 declare var Response: {
~~~~~~~~
node_modules/typescript/lib/lib.dom.d.ts:12463:13
12463 declare var Response: {
~~~~~~~~
'Response' was also declared here.
node_modules/@types/react-native/globals.d.ts:253:14 - error TS2300: Duplicate identifier 'XMLHttpRequestResponseType'.
253 declare type XMLHttpRequestResponseType = "" | "arraybuffer" | "blob" | "document" | "json" | "text";
~~~~~~~~~~~~~~~~~~~~~~~~~~
node_modules/typescript/lib/lib.dom.d.ts:18746:6
18746 type XMLHttpRequestResponseType = "" | "arraybuffer" | "blob" | "document" | "json" | "text";
~~~~~~~~~~~~~~~~~~~~~~~~~~
'XMLHttpRequestResponseType' was also declared here.
node_modules/@types/react-native/index.d.ts:9417:18 - error TS2717: Subsequent property declarations must have the same type. Property 'geolocation' must be of type 'Geolocation', but here has type 'GeolocationStatic'.
9417 readonly geolocation: Geolocation;
~~~~~~~~~~~
node_modules/typescript/lib/lib.dom.d.ts:10581:14
10581 readonly geolocation: Geolocation;
~~~~~~~~~~~
'geolocation' was also declared here.
node_modules/@types/react-native/index.d.ts:9420:11 - error TS2451: Cannot redeclare block-scoped variable 'navigator'.
9420 const navigator: Navigator;
~~~~~~~~~
node_modules/typescript/lib/lib.dom.d.ts:18152:13
18152 declare var navigator: Navigator;
~~~~~~~~~
'navigator' was also declared here.
node_modules/typescript/lib/lib.dom.d.ts:5353:11 - error TS2300: Duplicate identifier 'FormData'.
5353 interface FormData {
~~~~~~~~
node_modules/@types/react-native/globals.d.ts:40:15
40 declare class FormData {
~~~~~~~~
'FormData' was also declared here.
node_modules/typescript/lib/lib.dom.d.ts:5363:13 - error TS2300: Duplicate identifier 'FormData'.
5363 declare var FormData: {
~~~~~~~~
node_modules/@types/react-native/globals.d.ts:40:15
40 declare class FormData {
~~~~~~~~
'FormData' was also declared here.
node_modules/typescript/lib/lib.dom.d.ts:18152:13 - error TS2451: Cannot redeclare block-scoped variable 'navigator'.
18152 declare var navigator: Navigator;
~~~~~~~~~
node_modules/@types/react-native/index.d.ts:9420:11
9420 const navigator: Navigator;
~~~~~~~~~
'navigator' was also declared here.
node_modules/typescript/lib/lib.dom.d.ts:18568:6 - error TS2300: Duplicate identifier 'RequestInfo'.
18568 type RequestInfo = Request | string;
~~~~~~~~~~~
node_modules/@types/react-native/globals.d.ts:111:14
111 declare type RequestInfo = Request | string;
~~~~~~~~~~~
'RequestInfo' was also declared here.
node_modules/typescript/lib/lib.dom.d.ts:18746:6 - error TS2300: Duplicate identifier 'XMLHttpRequestResponseType'.
18746 type XMLHttpRequestResponseType = "" | "arraybuffer" | "blob" | "document" | "json" | "text";
~~~~~~~~~~~~~~~~~~~~~~~~~~
node_modules/@types/react-native/globals.d.ts:253:14
253 declare type XMLHttpRequestResponseType = "" | "arraybuffer" | "blob" | "document" | "json" | "text";
~~~~~~~~~~~~~~~~~~~~~~~~~~
'XMLHttpRequestResponseType' was also declared here.
Found 12 errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only other thought before hearing from Daniel is maybe adding the @types/react-native
path to exclude for the browser tsconfig would work if the consensus was to split into separate tsconfig files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use separate tsconfig files for the browser and native files... Separate them with
include
and/orexclude
properties.
I agree, and I'm more and more convinced that we'll want separate tsconfig files by package. #18942 is one example, and I think we'll see the advantages of fine-grained control over time.
It would be great to see that with test files and jest
as well. Being able to grab global describe
or it
anywhere in the project is a potential source of bugs.
Which type definitions are conflicting? Sounds like they are defining ambients that are conflicting with node's ambients? There must be a situation where this has affected some other projects as well, no?
I've experienced conflicts in another monorepo, and my conclusion was that explicitly defining types
becomes necessary. In the monorepo setups I'm familiar with —this one and Calypso— @types
are devDependencies
, and devDependencies
are always root dependencies.
When building multiple packages that target different platforms, it's not surprising that including all root @types
in all builds leads to problems and conflicts. The browser, node, and react-native are different platforms and different and conflicting types depending on our target platform seems desirable.
I'd be extremely cautious in specifying individual types by name because that could lead to a situation where, if someone were to install a new set of definitions without also including the name of those in the tsconfig, the bug would be difficult to pinpoint.
Additionally, I'm not sure how this would play with definitions that depend on other definitions. You would have to include those as well in your list of types. (i.e., this doesn't scale particularly well).
My (limited) experience has been that few type declarations are ambient/global, and they should be treated with care. Most package types will be included by explicitly importing the associated module.
I'm a bit surprised by the inclusion of react
in this list. Maybe a simpler and more portable JSX
import could be used? I'd expect react
imports to be included when including @wordpress/element
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've excluded react-native
packages from this tsconfig, eventually we'll have our own tsconfig as we port our code from flow 👍
In the meantime, are we ok to keep those types in? I did not want to introduce a new JSX
type here as @types/react
is already installed as a dependency but I'm happy to help revisit this in another PR
@@ -16,16 +16,11 @@ module.exports = { | |||
"react", | |||
"react-native", | |||
"jest", | |||
"flowtype" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't converted flow files to esnext yet but I just wanted to have less dependencies to deal with
LGTM :) |
}, | ||
"resolutions": { | ||
"@react-native-community/cli": "^1.5.2" | ||
"scripts": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar question, how do you plan to work with this package given that there is a huge number of scripts? Some of them reference the local node_modules
folder but others go two levels up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure yet, that's something we can discuss here or on follow up PRs when we tackle this problem. I think that given the amount of scripts we might want to keep those here for now as to not clutter the main package.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of them reference the local node_modules folder but others go two levels up.
In theory there should not be any local node_modules
folder, since that indicates a conflict. We'll work to fix those conflicts.
@@ -9,6 +9,7 @@ | |||
"noEmit": true, | |||
"resolveJsonModule": true, | |||
"target": "esnext", | |||
"types": ["react", "node", "jest", "requestidlecallback"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.map( ( packageName ) => packageName.replace( WORDPRESS_NAMESPACE, '' ) ); | ||
.map( ( packageName ) => packageName.replace( WORDPRESS_NAMESPACE, '' ) ) | ||
// exclude react-native packages from the main build | ||
.filter( ( packageName ) => ! packageName.startsWith( 'react-native' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the meantime, @youknowriad introduced a list of bundled packages that don't get included:
Lines 29 to 37 in feed14a
const BUNDLED_PACKAGES = [ '@wordpress/icons' ]; | |
const gutenbergPackages = Object.keys( dependencies ) | |
.filter( | |
( packageName ) => | |
! BUNDLED_PACKAGES.includes( packageName ) && | |
packageName.startsWith( WORDPRESS_NAMESPACE ) | |
) | |
.map( ( packageName ) => packageName.replace( WORDPRESS_NAMESPACE, '' ) ); |
Closer to merge, we should consolidate it somehow.
"peerDependencies": { | ||
"react-native": "0.59.0", | ||
"react-native-windows": "0.54.0" | ||
"react-native": "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should react
be listed here as well? I see it listed in another package. Given that RN
depends on React, we probably can remove it on another package, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to be honest, we haven't used or thought about react-native-bridge
as a standalone package. It's still tightly coupled with the main native editor, thus the reason why I replaced the version with "*"
here, so we can revisit later.
We could indeed add react
here, but I wonder if it would change anything in termes of packages that use this bridge dependency, since react
is already a peer dependency of react-native
it should display an error or a warning if react is not a dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed react
is a dependency of react-native-aztec
, so it could probably be removed there. Let's revisit peerDependencies later if that's ok with you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, I just flagged items to go through during the final iteration 👍
Description
This is a part of the migration of gutenberg-mobile to the gutenberg repo. See #18508
In this PR we reorganize dependencies related to gutenberg-mobile.
devDependendencies
inpackages/react-native-editor/package.json
since we have a lint script in gutenberg that makes sur no package hasdevDependencies
. We don't want to pollute the rootpackage.json
too much for now though as many of those are used for e2e tests and have a non-compatible Apache 2.0 license. We'll want to address that with a separatepackages/react-native-e2e-test
package.package.json
. This is the big part, as it literally integrates our react-native libs into the project. For this to work without errors we need to exclude them from the lint, build and tests.gutenberg
usingnpm run native
. For instance:yarn start
=>npm run native start
We also get rid of unused files and cleaned up a bit. See list of changes.
List of changes
packages/react-native-editor/gutenberg
packages/react-native-editor/symlinked-packages
packages/react-native-editor/symlinked-packages-in-parent
gutenberg
:@wordpress/react-native-aztec
,@wordpress/react-native-bridge
ans@wordpress/react-native-editor
and add those as dependencies in the mainpackage.json
@types/node
dependency as we require node.js modules for the i18n script (will be updated later) andnpm run lint-types
check those imports.jest
dependency asnpm run test-unit:native
is currently failing without itreact-native
version to target the same version used inpackages/react-native-editor
react-native-aztec
in the code with@wordpress/react-native-aztec
react-native-gutenberg-bridge
in the code with@wordpress/react-native-bridge
devDependencies
frompackages/react-native-editor/package.json
todependencies
as they are currently required for our build to work properly. Many of those are required for e2e testing and will be moved to it's own packagepackages/react-native-e2e-tests
later.packages/react-native-editor/package.json
andpackages/react-native-aztec/package.json
to usenpm
instead of yarn.packages/react-native-*
from the linter in.eslintignore
packages/react-native-*
from the build inbin/packages/build.js
How has this been tested?
npm install
should install all dependenciesnpm run native ...
should run a command fromreact-native-editor
package. Building and running the app is still not possible at this stagenpm run native test
should run fine (test may fail because of the environment, need to default to android)npm run test-unit:native
should passnpm run lint
should pass. Note that native packages are currently excluded from the linter. We can fix those errors before switching to gutenberg but then it will be harder to merge changes from gutenberg-mobile during the monorepo workTypes of changes
Move dependencies to root package.json from imported packages.
Checklist: