Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Deps] pin
hash-base to ~3.0, due to a breaking change
- Loading branch information
9e2bf12There 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.
@ljharb Why have you changed readable-stream version here? This is causing issues with
crypto-browserifyinbrowserify-sign.9e2bf12There 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.
It was necessary to restore older node compat. See #86 (comment) - the existence of
processwon’t cause any issues with a working bundler.9e2bf12There 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.
Expect when it's used in React Native/Metro where
processis not available.Personally, I don't think this change is worth. Adding compat support of very old Node version is causing major issues on other packages. We had to pin resolution to v4.2.2 to avoid all sort of issues in React Native.
@ljharb
9e2bf12There 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.
Then that just means that Metro is broken.
9e2bf12There 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.
can you tell me how to solve it？I use
browserify-sign+4.2.3.patch
but it has no effect.
@rjblopes
9e2bf12There 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.
Just FYI. This indeed causes a regression on React Native where the process variable does not exist. People are patching it manually, thread here:
margelo/react-native-quick-crypto#242
Metro is not broken. It's just a runtime that does not have all the environment variables as the browser.
9e2bf12There 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.
@ospfranco if it's meant to consume node modules, and lacks
process, it is broken. metro isn't a runtime, as i understand it, it's a bundler.9e2bf12There 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.
Sorry, wrote that in a hurry. Yes, metro is the bundler, has nothing to do with the current issue at hand. I meant RN as a runtime (internally uses JSC or Hermes but does not implement the full range of web APIs) is lacking the same APIs as web and node, so
processis missing and probably won't be added any time soon. Would you be willing to revert this change or Node has priority for you? If you are not willing to revert this a large amount of users who depend on crypto-browserify (directly or indirectly) will come with the same request anyway, so we can at least patch it in our own modules manually.9e2bf12There 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.
Reverting this breaks node < 4. RN is using a build tool, so there should be many options available for RN users to support it.
9e2bf12There 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.
:S who uses Node 4?
The amount of work a release of React Native needs is massive and polyfilling a web api automatically will be hard if not impossible. I can already see your mind is set on this though. We might as well patch it however way we can. Thanks anyways.
9e2bf12There 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.
processisn't a web API, it's a node api, and a maximal polyfill already exists https://www.npmjs.com/package/process