-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: add react-native polyfills #1915
Conversation
b5e8b17
to
359b197
Compare
size-limit report 📦
|
@@ -5,7 +5,7 @@ jobs: | |||
pre-release: | |||
name: pre-release | |||
runs-on: ubuntu-latest | |||
if: github.event_name == 'workflow_dispatch' && github.ref == 'refs/heads/master' | |||
if: github.event_name == 'workflow_dispatch' |
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.
side change to allow publishing from other branches
packages/polyfills/package.json
Outdated
@@ -0,0 +1,77 @@ | |||
{ | |||
"name": "@waku/polyfills", |
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.
@waku-org/js-waku-developers how do you like the name? It is intended for ReactNative
polyfilling.
I'd like to avoid long two worded names if possible.
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.
ah, considering it's just for react-native, i would personally prefer to have that part of the package name for max verbosity
maybe in the future when we have different kinds of polyfills, this name would make more sense :D
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.
renamed to react-native-polyfills
packages/polyfills/CHANGELOG.md
Outdated
@@ -0,0 +1 @@ | |||
# Changelog |
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.
nit: do we need this file? 🤔
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.
we do as it gets populated by release-please
bot
e.g https://github.com/waku-org/js-waku/blob/master/packages/sdk/CHANGELOG.md
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.
sorry i might not understand this, but: where are the poylfills? :P
packages/polyfills/src/metro.ts
Outdated
// eslint-disable-next-line @typescript-eslint/no-var-requires | ||
const path = require("path"); |
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.
any reason we aren't doing es6 imports?
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.
no in particular, es6 is longer so I usually prefer require
style
changed here to see if it would work with metro
packages/polyfills/package.json
Outdated
], | ||
"scripts": { | ||
"build": "run-s build:**", | ||
"build:esm": "tsc", |
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.
important to note that rollup
will not be bundling this package as bundling not needed and should be figured out by react-native
application
Problem
ReactNative
can use two types of bundler:Following dependencies were a cause of failure for
ReactNative
app:Solution
Metro bundler is still not compatible by default with exports map so
unstable_enablePackageExports
should be enabled explicitly.As for problems with polyfills - this package addresses it.