fix: Remove Flow from @react-native/assets-registry - registry.js

[codinsonn]

Summary

React native shipping Flow types breaks universal apps

In essence:

Next.js does not transpile Flow, yet combining Expo with Next.js is becoming a more common combo

Related issues and Github comments:

• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Module not found: Can't resolve '@react-native/assets-registry/registry' expo/expo#21469 (comment)

It might be a better idea to rewrite the entire @react-native/assets-registry in Typescript instead of Flow. I'm willing to do this myself if you're open for that. But really, as long as Flow goes, it will no longer break universal app setups with Next.js

Changelog:

[GENERAL] [REMOVED] - Fixed @react-native/assets-registry breaking in Next.js due to Flow types

Test Plan

Reproducible by starting a new expo + next.js and adding + using e.g. @bacons/mdx or @expo/vector-icons on the web with Next.js 13+

Fix testable by applying the following patch-package diff:

expo/expo#21623 (comment)

@react-native+assets+1.0.0.patch

diff --git a/node_modules/@react-native/assets/registry.js b/node_modules/@react-native/assets/registry.js
index 088187f..0a384d1 100644
--- a/node_modules/@react-native/assets/registry.js+++ b/node_modules/@react-native/assets/registry.js@@ -10,28 +10,16 @@
'use strict';
-export type PackagerAsset = {- +__packager_asset: boolean,- +fileSystemLocation: string,- +httpServerLocation: string,- +width: ?number,- +height: ?number,- +scales: Array<number>,- +hash: string,- +name: string,- +type: string,- ...-};-const assets: Array<PackagerAsset> = [];+const assets = [];-function registerAsset(asset: PackagerAsset): number {+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {+function getAssetByID(assetId) {
return assets[assetId - 1];
}

[facebook-github-bot]

Hi @codinsonn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[codinsonn]

To be fair, if anyone from the Facebook team or React-Native community is willing to spearhead a move away from Flow to Typescript (for at least @react-native/assets-registry), that might be a better solution than merging this PR.

(In which case, feel free to close this one)

[karpolan]

Will somebody fix it? When?!

[headfire94]

@codinsonn this PR can't be merged, see tests failing because other packages depend on flow types from this one. seems like bigger refactoring needed

[necolas]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

Ok, I did it myself:

/patches/@react-native+assets-registry+0.76.1.patch

diff --git a/node_modules/@react-native/assets-registry/registry.js b/node_modules/@react-native/assets-registry/registry.js
index 64b2735..d31da85 100644
--- a/node_modules/@react-native/assets-registry/registry.js
+++ b/node_modules/@react-native/assets-registry/registry.js
@@ -4,38 +4,39 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
- * @flow strict
- * @format
*/
'use strict';
-export type AssetDestPathResolver = 'android' | 'generic';
+// export type AssetDestPathResolver = 'android' | 'generic';
-export type PackagerAsset = {
- +__packager_asset: boolean,
- +fileSystemLocation: string,
- +httpServerLocation: string,
- +width: ?number,
- +height: ?number,
- +scales: Array<number>,
- +hash: string,
- +name: string,
- +type: string,
- +resolver?: AssetDestPathResolver,
- ...
-};
+// export type PackagerAsset = {
+// +__packager_asset: boolean,
+// +fileSystemLocation: string,
+// +httpServerLocation: string,
+// +width: ?number,
+// +height: ?number,
+// +scales: Array<number>,
+// +hash: string,
+// +name: string,
+// +type: string,
+// +resolver?: AssetDestPathResolver,
+// ...
+// };
-const assets: Array<PackagerAsset> = [];
+// const assets: Array<PackagerAsset> = [];
+const assets = [];
-function registerAsset(asset: PackagerAsset): number {
+// function registerAsset(asset: PackagerAsset): number {
+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {
+// function getAssetByID(assetId: number): PackagerAsset {
+function getAssetByID(assetId) {
return assets[assetId - 1];
}
-module.exports = {registerAsset, getAssetByID};
+module.exports = { registerAsset, getAssetByID };

[codinsonn]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

That does sound like a better solution than removing flow types or doing a bigger refactor.

I'm all for it.

Until then, I think I'll patch it out in my own projects. (thx @karpolan)

Maybe this PR should be converted into an issue instead?

[exzos28]

looks like it was merged here - #48458

[codinsonn]

Closing this as it seems to have been fixed by #48458 in 697e946 like @exzos28 mentioned

[codinsonn]

Summary

React native shipping Flow types breaks universal apps

In essence:

Next.js does not transpile Flow, yet combining Expo with Next.js is becoming a more common combo

Related issues and Github comments:

• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Module not found: Can't resolve '@react-native/assets-registry/registry' expo/expo#21469 (comment)

It might be a better idea to rewrite the entire @react-native/assets-registry in Typescript instead of Flow. I'm willing to do this myself if you're open for that. But really, as long as Flow goes, it will no longer break universal app setups with Next.js

Changelog:

[GENERAL] [REMOVED] - Fixed @react-native/assets-registry breaking in Next.js due to Flow types

Test Plan

Reproducible by starting a new expo + next.js and adding + using e.g. @bacons/mdx or @expo/vector-icons on the web with Next.js 13+

Fix testable by applying the following patch-package diff:

expo/expo#21623 (comment)

@react-native+assets+1.0.0.patch

diff --git a/node_modules/@react-native/assets/registry.js b/node_modules/@react-native/assets/registry.js
index 088187f..0a384d1 100644
--- a/node_modules/@react-native/assets/registry.js+++ b/node_modules/@react-native/assets/registry.js@@ -10,28 +10,16 @@
'use strict';
-export type PackagerAsset = {- +__packager_asset: boolean,- +fileSystemLocation: string,- +httpServerLocation: string,- +width: ?number,- +height: ?number,- +scales: Array<number>,- +hash: string,- +name: string,- +type: string,- ...-};-const assets: Array<PackagerAsset> = [];+const assets = [];-function registerAsset(asset: PackagerAsset): number {+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {+function getAssetByID(assetId) {
return assets[assetId - 1];
}

[facebook-github-bot]

Hi @codinsonn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[codinsonn]

To be fair, if anyone from the Facebook team or React-Native community is willing to spearhead a move away from Flow to Typescript (for at least @react-native/assets-registry), that might be a better solution than merging this PR.

(In which case, feel free to close this one)

[karpolan]

Will somebody fix it? When?!

[headfire94]

@codinsonn this PR can't be merged, see tests failing because other packages depend on flow types from this one. seems like bigger refactoring needed

[necolas]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

Ok, I did it myself:

/patches/@react-native+assets-registry+0.76.1.patch

diff --git a/node_modules/@react-native/assets-registry/registry.js b/node_modules/@react-native/assets-registry/registry.js
index 64b2735..d31da85 100644
--- a/node_modules/@react-native/assets-registry/registry.js
+++ b/node_modules/@react-native/assets-registry/registry.js
@@ -4,38 +4,39 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
- * @flow strict
- * @format
*/
'use strict';
-export type AssetDestPathResolver = 'android' | 'generic';
+// export type AssetDestPathResolver = 'android' | 'generic';
-export type PackagerAsset = {
- +__packager_asset: boolean,
- +fileSystemLocation: string,
- +httpServerLocation: string,
- +width: ?number,
- +height: ?number,
- +scales: Array<number>,
- +hash: string,
- +name: string,
- +type: string,
- +resolver?: AssetDestPathResolver,
- ...
-};
+// export type PackagerAsset = {
+// +__packager_asset: boolean,
+// +fileSystemLocation: string,
+// +httpServerLocation: string,
+// +width: ?number,
+// +height: ?number,
+// +scales: Array<number>,
+// +hash: string,
+// +name: string,
+// +type: string,
+// +resolver?: AssetDestPathResolver,
+// ...
+// };
-const assets: Array<PackagerAsset> = [];
+// const assets: Array<PackagerAsset> = [];
+const assets = [];
-function registerAsset(asset: PackagerAsset): number {
+// function registerAsset(asset: PackagerAsset): number {
+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {
+// function getAssetByID(assetId: number): PackagerAsset {
+function getAssetByID(assetId) {
return assets[assetId - 1];
}
-module.exports = {registerAsset, getAssetByID};
+module.exports = { registerAsset, getAssetByID };

[codinsonn]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

That does sound like a better solution than removing flow types or doing a bigger refactor.

I'm all for it.

Until then, I think I'll patch it out in my own projects. (thx @karpolan)

Maybe this PR should be converted into an issue instead?

[exzos28]

looks like it was merged here - #48458

[codinsonn]

Closing this as it seems to have been fixed by #48458 in 697e946 like @exzos28 mentioned

[codinsonn]

Summary

React native shipping Flow types breaks universal apps

In essence:

Next.js does not transpile Flow, yet combining Expo with Next.js is becoming a more common combo

Related issues and Github comments:

• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Module not found: Can't resolve '@react-native/assets-registry/registry' expo/expo#21469 (comment)

It might be a better idea to rewrite the entire @react-native/assets-registry in Typescript instead of Flow. I'm willing to do this myself if you're open for that. But really, as long as Flow goes, it will no longer break universal app setups with Next.js

Changelog:

[GENERAL] [REMOVED] - Fixed @react-native/assets-registry breaking in Next.js due to Flow types

Test Plan

Reproducible by starting a new expo + next.js and adding + using e.g. @bacons/mdx or @expo/vector-icons on the web with Next.js 13+

Fix testable by applying the following patch-package diff:

expo/expo#21623 (comment)

@react-native+assets+1.0.0.patch

diff --git a/node_modules/@react-native/assets/registry.js b/node_modules/@react-native/assets/registry.js
index 088187f..0a384d1 100644
--- a/node_modules/@react-native/assets/registry.js+++ b/node_modules/@react-native/assets/registry.js@@ -10,28 +10,16 @@
'use strict';
-export type PackagerAsset = {- +__packager_asset: boolean,- +fileSystemLocation: string,- +httpServerLocation: string,- +width: ?number,- +height: ?number,- +scales: Array<number>,- +hash: string,- +name: string,- +type: string,- ...-};-const assets: Array<PackagerAsset> = [];+const assets = [];-function registerAsset(asset: PackagerAsset): number {+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {+function getAssetByID(assetId) {
return assets[assetId - 1];
}

[facebook-github-bot]

Hi @codinsonn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[codinsonn]

To be fair, if anyone from the Facebook team or React-Native community is willing to spearhead a move away from Flow to Typescript (for at least @react-native/assets-registry), that might be a better solution than merging this PR.

(In which case, feel free to close this one)

[karpolan]

Will somebody fix it? When?!

[headfire94]

@codinsonn this PR can't be merged, see tests failing because other packages depend on flow types from this one. seems like bigger refactoring needed

[necolas]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

Ok, I did it myself:

/patches/@react-native+assets-registry+0.76.1.patch

diff --git a/node_modules/@react-native/assets-registry/registry.js b/node_modules/@react-native/assets-registry/registry.js
index 64b2735..d31da85 100644
--- a/node_modules/@react-native/assets-registry/registry.js
+++ b/node_modules/@react-native/assets-registry/registry.js
@@ -4,38 +4,39 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
- * @flow strict
- * @format
*/
'use strict';
-export type AssetDestPathResolver = 'android' | 'generic';
+// export type AssetDestPathResolver = 'android' | 'generic';
-export type PackagerAsset = {
- +__packager_asset: boolean,
- +fileSystemLocation: string,
- +httpServerLocation: string,
- +width: ?number,
- +height: ?number,
- +scales: Array<number>,
- +hash: string,
- +name: string,
- +type: string,
- +resolver?: AssetDestPathResolver,
- ...
-};
+// export type PackagerAsset = {
+// +__packager_asset: boolean,
+// +fileSystemLocation: string,
+// +httpServerLocation: string,
+// +width: ?number,
+// +height: ?number,
+// +scales: Array<number>,
+// +hash: string,
+// +name: string,
+// +type: string,
+// +resolver?: AssetDestPathResolver,
+// ...
+// };
-const assets: Array<PackagerAsset> = [];
+// const assets: Array<PackagerAsset> = [];
+const assets = [];
-function registerAsset(asset: PackagerAsset): number {
+// function registerAsset(asset: PackagerAsset): number {
+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {
+// function getAssetByID(assetId: number): PackagerAsset {
+function getAssetByID(assetId) {
return assets[assetId - 1];
}
-module.exports = {registerAsset, getAssetByID};
+module.exports = { registerAsset, getAssetByID };

[codinsonn]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

That does sound like a better solution than removing flow types or doing a bigger refactor.

I'm all for it.

Until then, I think I'll patch it out in my own projects. (thx @karpolan)

Maybe this PR should be converted into an issue instead?

[exzos28]

looks like it was merged here - #48458

[codinsonn]

Closing this as it seems to have been fixed by #48458 in 697e946 like @exzos28 mentioned

[codinsonn]

Summary

React native shipping Flow types breaks universal apps

In essence:

Next.js does not transpile Flow, yet combining Expo with Next.js is becoming a more common combo

Related issues and Github comments:

• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Module not found: Can't resolve '@react-native/assets-registry/registry' expo/expo#21469 (comment)

It might be a better idea to rewrite the entire @react-native/assets-registry in Typescript instead of Flow. I'm willing to do this myself if you're open for that. But really, as long as Flow goes, it will no longer break universal app setups with Next.js

Changelog:

[GENERAL] [REMOVED] - Fixed @react-native/assets-registry breaking in Next.js due to Flow types

Test Plan

Reproducible by starting a new expo + next.js and adding + using e.g. @bacons/mdx or @expo/vector-icons on the web with Next.js 13+

Fix testable by applying the following patch-package diff:

expo/expo#21623 (comment)

@react-native+assets+1.0.0.patch

diff --git a/node_modules/@react-native/assets/registry.js b/node_modules/@react-native/assets/registry.js
index 088187f..0a384d1 100644
--- a/node_modules/@react-native/assets/registry.js+++ b/node_modules/@react-native/assets/registry.js@@ -10,28 +10,16 @@
'use strict';
-export type PackagerAsset = {- +__packager_asset: boolean,- +fileSystemLocation: string,- +httpServerLocation: string,- +width: ?number,- +height: ?number,- +scales: Array<number>,- +hash: string,- +name: string,- +type: string,- ...-};-const assets: Array<PackagerAsset> = [];+const assets = [];-function registerAsset(asset: PackagerAsset): number {+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {+function getAssetByID(assetId) {
return assets[assetId - 1];
}

[facebook-github-bot]

Hi @codinsonn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[codinsonn]

To be fair, if anyone from the Facebook team or React-Native community is willing to spearhead a move away from Flow to Typescript (for at least @react-native/assets-registry), that might be a better solution than merging this PR.

(In which case, feel free to close this one)

[karpolan]

Will somebody fix it? When?!

[headfire94]

@codinsonn this PR can't be merged, see tests failing because other packages depend on flow types from this one. seems like bigger refactoring needed

[necolas]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

Ok, I did it myself:

/patches/@react-native+assets-registry+0.76.1.patch

diff --git a/node_modules/@react-native/assets-registry/registry.js b/node_modules/@react-native/assets-registry/registry.js
index 64b2735..d31da85 100644
--- a/node_modules/@react-native/assets-registry/registry.js
+++ b/node_modules/@react-native/assets-registry/registry.js
@@ -4,38 +4,39 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
- * @flow strict
- * @format
*/
'use strict';
-export type AssetDestPathResolver = 'android' | 'generic';
+// export type AssetDestPathResolver = 'android' | 'generic';
-export type PackagerAsset = {
- +__packager_asset: boolean,
- +fileSystemLocation: string,
- +httpServerLocation: string,
- +width: ?number,
- +height: ?number,
- +scales: Array<number>,
- +hash: string,
- +name: string,
- +type: string,
- +resolver?: AssetDestPathResolver,
- ...
-};
+// export type PackagerAsset = {
+// +__packager_asset: boolean,
+// +fileSystemLocation: string,
+// +httpServerLocation: string,
+// +width: ?number,
+// +height: ?number,
+// +scales: Array<number>,
+// +hash: string,
+// +name: string,
+// +type: string,
+// +resolver?: AssetDestPathResolver,
+// ...
+// };
-const assets: Array<PackagerAsset> = [];
+// const assets: Array<PackagerAsset> = [];
+const assets = [];
-function registerAsset(asset: PackagerAsset): number {
+// function registerAsset(asset: PackagerAsset): number {
+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {
+// function getAssetByID(assetId: number): PackagerAsset {
+function getAssetByID(assetId) {
return assets[assetId - 1];
}
-module.exports = {registerAsset, getAssetByID};
+module.exports = { registerAsset, getAssetByID };

[codinsonn]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

That does sound like a better solution than removing flow types or doing a bigger refactor.

I'm all for it.

Until then, I think I'll patch it out in my own projects. (thx @karpolan)

Maybe this PR should be converted into an issue instead?

[exzos28]

looks like it was merged here - #48458

[codinsonn]

Closing this as it seems to have been fixed by #48458 in 697e946 like @exzos28 mentioned

[codinsonn]

Summary

React native shipping Flow types breaks universal apps

In essence:

Next.js does not transpile Flow, yet combining Expo with Next.js is becoming a more common combo

Related issues and Github comments:

• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Module not found: Can't resolve '@react-native/assets-registry/registry' expo/expo#21469 (comment)

It might be a better idea to rewrite the entire @react-native/assets-registry in Typescript instead of Flow. I'm willing to do this myself if you're open for that. But really, as long as Flow goes, it will no longer break universal app setups with Next.js

Changelog:

[GENERAL] [REMOVED] - Fixed @react-native/assets-registry breaking in Next.js due to Flow types

Test Plan

Reproducible by starting a new expo + next.js and adding + using e.g. @bacons/mdx or @expo/vector-icons on the web with Next.js 13+

Fix testable by applying the following patch-package diff:

expo/expo#21623 (comment)

@react-native+assets+1.0.0.patch

diff --git a/node_modules/@react-native/assets/registry.js b/node_modules/@react-native/assets/registry.js
index 088187f..0a384d1 100644
--- a/node_modules/@react-native/assets/registry.js+++ b/node_modules/@react-native/assets/registry.js@@ -10,28 +10,16 @@
'use strict';
-export type PackagerAsset = {- +__packager_asset: boolean,- +fileSystemLocation: string,- +httpServerLocation: string,- +width: ?number,- +height: ?number,- +scales: Array<number>,- +hash: string,- +name: string,- +type: string,- ...-};-const assets: Array<PackagerAsset> = [];+const assets = [];-function registerAsset(asset: PackagerAsset): number {+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {+function getAssetByID(assetId) {
return assets[assetId - 1];
}

[facebook-github-bot]

Hi @codinsonn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[codinsonn]

To be fair, if anyone from the Facebook team or React-Native community is willing to spearhead a move away from Flow to Typescript (for at least @react-native/assets-registry), that might be a better solution than merging this PR.

(In which case, feel free to close this one)

[karpolan]

Will somebody fix it? When?!

[headfire94]

@codinsonn this PR can't be merged, see tests failing because other packages depend on flow types from this one. seems like bigger refactoring needed

[necolas]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

Ok, I did it myself:

/patches/@react-native+assets-registry+0.76.1.patch

diff --git a/node_modules/@react-native/assets-registry/registry.js b/node_modules/@react-native/assets-registry/registry.js
index 64b2735..d31da85 100644
--- a/node_modules/@react-native/assets-registry/registry.js
+++ b/node_modules/@react-native/assets-registry/registry.js
@@ -4,38 +4,39 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
- * @flow strict
- * @format
*/
'use strict';
-export type AssetDestPathResolver = 'android' | 'generic';
+// export type AssetDestPathResolver = 'android' | 'generic';
-export type PackagerAsset = {
- +__packager_asset: boolean,
- +fileSystemLocation: string,
- +httpServerLocation: string,
- +width: ?number,
- +height: ?number,
- +scales: Array<number>,
- +hash: string,
- +name: string,
- +type: string,
- +resolver?: AssetDestPathResolver,
- ...
-};
+// export type PackagerAsset = {
+// +__packager_asset: boolean,
+// +fileSystemLocation: string,
+// +httpServerLocation: string,
+// +width: ?number,
+// +height: ?number,
+// +scales: Array<number>,
+// +hash: string,
+// +name: string,
+// +type: string,
+// +resolver?: AssetDestPathResolver,
+// ...
+// };
-const assets: Array<PackagerAsset> = [];
+// const assets: Array<PackagerAsset> = [];
+const assets = [];
-function registerAsset(asset: PackagerAsset): number {
+// function registerAsset(asset: PackagerAsset): number {
+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {
+// function getAssetByID(assetId: number): PackagerAsset {
+function getAssetByID(assetId) {
return assets[assetId - 1];
}
-module.exports = {registerAsset, getAssetByID};
+module.exports = { registerAsset, getAssetByID };

[codinsonn]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

That does sound like a better solution than removing flow types or doing a bigger refactor.

I'm all for it.

Until then, I think I'll patch it out in my own projects. (thx @karpolan)

Maybe this PR should be converted into an issue instead?

[exzos28]

looks like it was merged here - #48458

[codinsonn]

Closing this as it seems to have been fixed by #48458 in 697e946 like @exzos28 mentioned

[codinsonn]

Summary

React native shipping Flow types breaks universal apps

In essence:

Next.js does not transpile Flow, yet combining Expo with Next.js is becoming a more common combo

Related issues and Github comments:

• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Module not found: Can't resolve '@react-native/assets-registry/registry' expo/expo#21469 (comment)

It might be a better idea to rewrite the entire @react-native/assets-registry in Typescript instead of Flow. I'm willing to do this myself if you're open for that. But really, as long as Flow goes, it will no longer break universal app setups with Next.js

Changelog:

[GENERAL] [REMOVED] - Fixed @react-native/assets-registry breaking in Next.js due to Flow types

Test Plan

Reproducible by starting a new expo + next.js and adding + using e.g. @bacons/mdx or @expo/vector-icons on the web with Next.js 13+

Fix testable by applying the following patch-package diff:

expo/expo#21623 (comment)

@react-native+assets+1.0.0.patch

diff --git a/node_modules/@react-native/assets/registry.js b/node_modules/@react-native/assets/registry.js
index 088187f..0a384d1 100644
--- a/node_modules/@react-native/assets/registry.js+++ b/node_modules/@react-native/assets/registry.js@@ -10,28 +10,16 @@
'use strict';
-export type PackagerAsset = {- +__packager_asset: boolean,- +fileSystemLocation: string,- +httpServerLocation: string,- +width: ?number,- +height: ?number,- +scales: Array<number>,- +hash: string,- +name: string,- +type: string,- ...-};-const assets: Array<PackagerAsset> = [];+const assets = [];-function registerAsset(asset: PackagerAsset): number {+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {+function getAssetByID(assetId) {
return assets[assetId - 1];
}

[facebook-github-bot]

Hi @codinsonn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[codinsonn]

To be fair, if anyone from the Facebook team or React-Native community is willing to spearhead a move away from Flow to Typescript (for at least @react-native/assets-registry), that might be a better solution than merging this PR.

(In which case, feel free to close this one)

[karpolan]

Will somebody fix it? When?!

[headfire94]

@codinsonn this PR can't be merged, see tests failing because other packages depend on flow types from this one. seems like bigger refactoring needed

[necolas]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

Ok, I did it myself:

/patches/@react-native+assets-registry+0.76.1.patch

diff --git a/node_modules/@react-native/assets-registry/registry.js b/node_modules/@react-native/assets-registry/registry.js
index 64b2735..d31da85 100644
--- a/node_modules/@react-native/assets-registry/registry.js
+++ b/node_modules/@react-native/assets-registry/registry.js
@@ -4,38 +4,39 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
- * @flow strict
- * @format
*/
'use strict';
-export type AssetDestPathResolver = 'android' | 'generic';
+// export type AssetDestPathResolver = 'android' | 'generic';
-export type PackagerAsset = {
- +__packager_asset: boolean,
- +fileSystemLocation: string,
- +httpServerLocation: string,
- +width: ?number,
- +height: ?number,
- +scales: Array<number>,
- +hash: string,
- +name: string,
- +type: string,
- +resolver?: AssetDestPathResolver,
- ...
-};
+// export type PackagerAsset = {
+// +__packager_asset: boolean,
+// +fileSystemLocation: string,
+// +httpServerLocation: string,
+// +width: ?number,
+// +height: ?number,
+// +scales: Array<number>,
+// +hash: string,
+// +name: string,
+// +type: string,
+// +resolver?: AssetDestPathResolver,
+// ...
+// };
-const assets: Array<PackagerAsset> = [];
+// const assets: Array<PackagerAsset> = [];
+const assets = [];
-function registerAsset(asset: PackagerAsset): number {
+// function registerAsset(asset: PackagerAsset): number {
+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {
+// function getAssetByID(assetId: number): PackagerAsset {
+function getAssetByID(assetId) {
return assets[assetId - 1];
}
-module.exports = {registerAsset, getAssetByID};
+module.exports = { registerAsset, getAssetByID };

[codinsonn]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

That does sound like a better solution than removing flow types or doing a bigger refactor.

I'm all for it.

Until then, I think I'll patch it out in my own projects. (thx @karpolan)

Maybe this PR should be converted into an issue instead?

[exzos28]

looks like it was merged here - #48458

[codinsonn]

Closing this as it seems to have been fixed by #48458 in 697e946 like @exzos28 mentioned

[codinsonn]

Summary

React native shipping Flow types breaks universal apps

In essence:

Next.js does not transpile Flow, yet combining Expo with Next.js is becoming a more common combo

Related issues and Github comments:

• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Module not found: Can't resolve '@react-native/assets-registry/registry' expo/expo#21469 (comment)

It might be a better idea to rewrite the entire @react-native/assets-registry in Typescript instead of Flow. I'm willing to do this myself if you're open for that. But really, as long as Flow goes, it will no longer break universal app setups with Next.js

Changelog:

[GENERAL] [REMOVED] - Fixed @react-native/assets-registry breaking in Next.js due to Flow types

Test Plan

Reproducible by starting a new expo + next.js and adding + using e.g. @bacons/mdx or @expo/vector-icons on the web with Next.js 13+

Fix testable by applying the following patch-package diff:

expo/expo#21623 (comment)

@react-native+assets+1.0.0.patch

diff --git a/node_modules/@react-native/assets/registry.js b/node_modules/@react-native/assets/registry.js
index 088187f..0a384d1 100644
--- a/node_modules/@react-native/assets/registry.js+++ b/node_modules/@react-native/assets/registry.js@@ -10,28 +10,16 @@
'use strict';
-export type PackagerAsset = {- +__packager_asset: boolean,- +fileSystemLocation: string,- +httpServerLocation: string,- +width: ?number,- +height: ?number,- +scales: Array<number>,- +hash: string,- +name: string,- +type: string,- ...-};-const assets: Array<PackagerAsset> = [];+const assets = [];-function registerAsset(asset: PackagerAsset): number {+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {+function getAssetByID(assetId) {
return assets[assetId - 1];
}

[facebook-github-bot]

Hi @codinsonn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[codinsonn]

To be fair, if anyone from the Facebook team or React-Native community is willing to spearhead a move away from Flow to Typescript (for at least @react-native/assets-registry), that might be a better solution than merging this PR.

(In which case, feel free to close this one)

[karpolan]

Will somebody fix it? When?!

[headfire94]

@codinsonn this PR can't be merged, see tests failing because other packages depend on flow types from this one. seems like bigger refactoring needed

[necolas]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

Ok, I did it myself:

/patches/@react-native+assets-registry+0.76.1.patch

diff --git a/node_modules/@react-native/assets-registry/registry.js b/node_modules/@react-native/assets-registry/registry.js
index 64b2735..d31da85 100644
--- a/node_modules/@react-native/assets-registry/registry.js
+++ b/node_modules/@react-native/assets-registry/registry.js
@@ -4,38 +4,39 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
- * @flow strict
- * @format
*/
'use strict';
-export type AssetDestPathResolver = 'android' | 'generic';
+// export type AssetDestPathResolver = 'android' | 'generic';
-export type PackagerAsset = {
- +__packager_asset: boolean,
- +fileSystemLocation: string,
- +httpServerLocation: string,
- +width: ?number,
- +height: ?number,
- +scales: Array<number>,
- +hash: string,
- +name: string,
- +type: string,
- +resolver?: AssetDestPathResolver,
- ...
-};
+// export type PackagerAsset = {
+// +__packager_asset: boolean,
+// +fileSystemLocation: string,
+// +httpServerLocation: string,
+// +width: ?number,
+// +height: ?number,
+// +scales: Array<number>,
+// +hash: string,
+// +name: string,
+// +type: string,
+// +resolver?: AssetDestPathResolver,
+// ...
+// };
-const assets: Array<PackagerAsset> = [];
+// const assets: Array<PackagerAsset> = [];
+const assets = [];
-function registerAsset(asset: PackagerAsset): number {
+// function registerAsset(asset: PackagerAsset): number {
+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {
+// function getAssetByID(assetId: number): PackagerAsset {
+function getAssetByID(assetId) {
return assets[assetId - 1];
}
-module.exports = {registerAsset, getAssetByID};
+module.exports = { registerAsset, getAssetByID };

[codinsonn]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

That does sound like a better solution than removing flow types or doing a bigger refactor.

I'm all for it.

Until then, I think I'll patch it out in my own projects. (thx @karpolan)

Maybe this PR should be converted into an issue instead?

[exzos28]

looks like it was merged here - #48458

[codinsonn]

Closing this as it seems to have been fixed by #48458 in 697e946 like @exzos28 mentioned

[codinsonn]

Summary

React native shipping Flow types breaks universal apps

In essence:

Next.js does not transpile Flow, yet combining Expo with Next.js is becoming a more common combo

Related issues and Github comments:

• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Next.js 13 + Expo 48 error loading @expo/vector-icons expo/expo#21623 (comment)
• Module not found: Can't resolve '@react-native/assets-registry/registry' expo/expo#21469 (comment)

It might be a better idea to rewrite the entire @react-native/assets-registry in Typescript instead of Flow. I'm willing to do this myself if you're open for that. But really, as long as Flow goes, it will no longer break universal app setups with Next.js

Changelog:

[GENERAL] [REMOVED] - Fixed @react-native/assets-registry breaking in Next.js due to Flow types

Test Plan

Reproducible by starting a new expo + next.js and adding + using e.g. @bacons/mdx or @expo/vector-icons on the web with Next.js 13+

Fix testable by applying the following patch-package diff:

expo/expo#21623 (comment)

@react-native+assets+1.0.0.patch

diff --git a/node_modules/@react-native/assets/registry.js b/node_modules/@react-native/assets/registry.js
index 088187f..0a384d1 100644
--- a/node_modules/@react-native/assets/registry.js+++ b/node_modules/@react-native/assets/registry.js@@ -10,28 +10,16 @@
'use strict';
-export type PackagerAsset = {- +__packager_asset: boolean,- +fileSystemLocation: string,- +httpServerLocation: string,- +width: ?number,- +height: ?number,- +scales: Array<number>,- +hash: string,- +name: string,- +type: string,- ...-};-const assets: Array<PackagerAsset> = [];+const assets = [];-function registerAsset(asset: PackagerAsset): number {+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {+function getAssetByID(assetId) {
return assets[assetId - 1];
}

[facebook-github-bot]

Hi @codinsonn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[codinsonn]

To be fair, if anyone from the Facebook team or React-Native community is willing to spearhead a move away from Flow to Typescript (for at least @react-native/assets-registry), that might be a better solution than merging this PR.

(In which case, feel free to close this one)

[karpolan]

Will somebody fix it? When?!

[headfire94]

@codinsonn this PR can't be merged, see tests failing because other packages depend on flow types from this one. seems like bigger refactoring needed

[necolas]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

[karpolan]

Does somebody have patch for "react-native": "0.76.1" ?

Ok, I did it myself:

/patches/@react-native+assets-registry+0.76.1.patch

diff --git a/node_modules/@react-native/assets-registry/registry.js b/node_modules/@react-native/assets-registry/registry.js
index 64b2735..d31da85 100644
--- a/node_modules/@react-native/assets-registry/registry.js
+++ b/node_modules/@react-native/assets-registry/registry.js
@@ -4,38 +4,39 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
- * @flow strict
- * @format
*/
'use strict';
-export type AssetDestPathResolver = 'android' | 'generic';
+// export type AssetDestPathResolver = 'android' | 'generic';
-export type PackagerAsset = {
- +__packager_asset: boolean,
- +fileSystemLocation: string,
- +httpServerLocation: string,
- +width: ?number,
- +height: ?number,
- +scales: Array<number>,
- +hash: string,
- +name: string,
- +type: string,
- +resolver?: AssetDestPathResolver,
- ...
-};
+// export type PackagerAsset = {
+// +__packager_asset: boolean,
+// +fileSystemLocation: string,
+// +httpServerLocation: string,
+// +width: ?number,
+// +height: ?number,
+// +scales: Array<number>,
+// +hash: string,
+// +name: string,
+// +type: string,
+// +resolver?: AssetDestPathResolver,
+// ...
+// };
-const assets: Array<PackagerAsset> = [];
+// const assets: Array<PackagerAsset> = [];
+const assets = [];
-function registerAsset(asset: PackagerAsset): number {
+// function registerAsset(asset: PackagerAsset): number {
+function registerAsset(asset) {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
-function getAssetByID(assetId: number): PackagerAsset {
+// function getAssetByID(assetId: number): PackagerAsset {
+function getAssetByID(assetId) {
return assets[assetId - 1];
}
-module.exports = {registerAsset, getAssetByID};
+module.exports = { registerAsset, getAssetByID };

[codinsonn]

I don't think we want to remove the flow types from the source code, but when this package is published to npm it should not have type annotations. It looks normalize-colors simply puts the flow types in an accompanying file, so the entry is valid JS https://github.com/facebook/react-native/tree/HEAD/packages/normalize-color

That does sound like a better solution than removing flow types or doing a bigger refactor.

I'm all for it.

Until then, I think I'll patch it out in my own projects. (thx @karpolan)

Maybe this PR should be converted into an issue instead?

[exzos28]

looks like it was merged here - #48458

[codinsonn]

Closing this as it seems to have been fixed by #48458 in 697e946 like @exzos28 mentioned