Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reactnative-identicon@0.85.2 throwing "no identifiers allowed directly after numeric literal" #516

Closed
Tbaut opened this issue Aug 28, 2021 · 8 comments

Comments

@Tbaut
Copy link
Contributor

Tbaut commented Aug 28, 2021

Hello there,
I just upgraded reactnative-identicon from 0.84.3 to 0.85.2 and it makes troubles, it throws the following with no stack:

"no identifiers allowed directly after numeric literal"

image

Any hint where this could come from?

My other deps:

"@polkadot/api": "^5.6.1",
"@polkadot/reactnative-identicon": "^0.85.2",
"@polkadot/util": "^7.3.1",
"@polkadot/util-crypto": "^7.3.1"

Sticking with 0.84.3 works, but this gives conflicting libs, @polkadot/util 7.1.1 with 7.3.1 and @polkadot/wasm-crypto 4.1.2 with 4.2.1

@jacogr
Copy link
Member

jacogr commented Aug 29, 2021

Dang it. No idea, it must be a deps upgrade.

Found it as "No identifiers allowed directly after numeric literal" in - node_modules/@polkadot/dev/node_modules/prettier/esm/parser-meriyah.mjs

Nowhere else, so not sure how this ends up as a production issue.

Also found this - expo/expo#12042 - the weird thing is we have not added any new 1_000-like numbers anywhere, we have been using them for ages, but it does get compiled out in the build output. However it could be that something now doesn't compile down the way it used to. So I would guess we need to search the build outputs, see if we have these literals somewhere and then figure out why they end up in there?

@jacogr
Copy link
Member

jacogr commented Aug 29, 2021

I took a look at the common repo compiled/build outputs, all numeric separators have been removed by the compilation phase. (This is for util & util-crypto) So no idea where this could slip in and doesn't seem to be from the polkadot-js outputs, which all use the same build.

e.g. (only relevant lines...)

// input in common/packages/util/src/bn/consts.ts
export const BN_MILLION: BN = new BN(1_000_000);

// generated output in common/packages/util/build/bn/consts.cjs (cjs)
const BN_MILLION = new _bn.BN(1000000);

// generated output in common/packages/util/build/bn/consts.js (esm)
export const BN_MILLION = new BN(1000000);

Same applies for the node_modules/@polkadot/util/bn/consts.{js, cjs} files as installed for 7.3.1

Doing a search through the node_modules/@polkadot/* (as installed in this repo), also yields no hints, i.e. it seems fully removed.

EDIT: Actually doing a search through the whole node_modules/ folder (as used), doesn't yield suspects.

@Tbaut
Copy link
Contributor Author

Tbaut commented Aug 29, 2021

Thanks for your time Jaco. I've spent a couple hours on this as well, I still have no clue unfortunately. I could not find this error in the generated bundle. I've tried to force the resolution of prettier to certain versions, but this didn't change anything.

I just built an apk with this version and I can see that on a real device it actually works without issue.

edit: I checked the bundle and I can't find any "1_0" either, and can see as well

  var BN_MILLION = new _bn.BN(1000000);
  exports.BN_MILLION = BN_MILLION;
  var BN_BILLION = new _bn.BN(1000000000);

And there are thousands of results for [0-9]_[0-9] unfortunately..

@jacogr
Copy link
Member

jacogr commented Aug 29, 2021

Does expo use Babel? There is an explicit transform for these things. https://babeljs.io/docs/en/babel-plugin-transform-literals

My TL;DR atm is that I really don't think it is polkadot-js's fault here, although it seems suspicious with the versions introducing this. (Happy to be proven wrong and adjust if needed, but really cannot find that it is in our generated code here)

@Tbaut
Copy link
Contributor Author

Tbaut commented Aug 29, 2021

Oh here it is:

  257,22: 			lum[i] = (chan <= 0.039_28) ? chan / 12.92 : ((chan + 0.055) / 1.055) ** 2.4;`

in @polkadot/ui-shared/node_modules/color/index.js

which comes from color@^4.0.1 which is one of the deps that changed with the bump. Using a "resolutions": {"color":"^3.2.1}, solved temporarily the problem.

Does expo use Babel?

React-native doesn't use expo afaik? I will look at how I can add this plugin to make sure everything is compiled correctly.

As usual, thank you so much for you help and guidance.

@Tbaut Tbaut closed this as completed Aug 29, 2021
@Tbaut
Copy link
Contributor Author

Tbaut commented Aug 29, 2021

A followup in case this can help anyone, this issue has been discussed in the color repo here and in the metro repo here, and a PR was submitted to add the relevant babel plugin to metro-react-native-babel-preset. Until this has been merged, adding the plugin babel-plugin-proposal-numeric-separator manually does the trick.

edit: I realized that adding babel plugins for react native works like a normal js repo, in my case installing it and adding it to babel.config.js here

@jacogr jacogr mentioned this issue Aug 29, 2021
@jacogr
Copy link
Member

jacogr commented Aug 29, 2021

Bump is reverted in #517 (0.85.3). I'm worried that it has a negative impact on react-native and Vue for the time being. This certainly should be addressed at the tools level, but since we don't need a bump, we may as well stick with older a bit longer and bump again in the future.

https://github.com/polkadot-js/ui/releases/tag/v0.85.3

@polkadot-js-bot
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Sep 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants