-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
support old browsers without polyfills #982
support old browsers without polyfills #982
Conversation
e52434a
to
1f5cd3c
Compare
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.
Thanks for your contribution, one question regarding a change.
@@ -3,18 +3,16 @@ import PropTypes from "prop-types"; | |||
|
|||
import { asNumber } from "../../utils"; | |||
|
|||
const nums = new Set(["number", "integer"]); |
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.
Shouldn't this Set
be taken from core-js
? Or maybe the same change as above, using the polyfilled includes
from core-js
.
transform-runtime automatically ponyfills globals like Set Promise setImmediate and Object.assign |
Thanks! |
@graingert i've resolved the merge conflict, please let me know if this looks correct, before I merge it. Thanks! |
oh, wait, no, I borked it. Let me try again... |
@magopian let me take over |
…playground. (rjsf-team#207)" This reverts commit 4a773de.
0d10d98
to
a33a930
Compare
@@ -42,10 +42,10 @@ | |||
}, | |||
"dependencies": { | |||
"ajv": "^5.2.3", | |||
"babel-preset-env": "^1.7.0", |
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.
woops, I put this in the wrong place in my last pr, should have been a devDep
@magopian done |
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.
Ok, excellent, thanks
If I understand this change correctly, it should result in a minor version bump, since direct support for old browsers is a new (non-breaking) feature of the package 🙏 |
@dalgard sure, but erm... it's a bit too late now |
Semver says it's a minor bump only if new functionality is added to the API. The API hasn't changed, so it's a patch version. |
@graingert , could you please look at #991 and #1022 and tell me what you think? I am worried that we may have broken the way some of our users are including rjsf, but I'm not an expert in JS packaging so I'm hoping you can help. |
Ok I'll have a look tomorrow |
@glasserc I took a look - looks like it's a simple fix to the webpack babel-loader config to exclude babel loader on babel and core-js in webpack. This is why it works when people use the package from npm directly |
To elaborate on the above comment, I spoke with @graingert via IRC and he explained that he thought the problem was that we're running babel on all our dependencies, including the ones that provide ponyfills such as corejs and babel-runtime, and that this is due to a webpack misconfiguration. I'll try to fix it and open a PR today. |
@glasserc currently I think you only need to exclude It may be prudent to exclude all of node_modules and switch to a whitelist, however |
Reasons for making this change
Currently react-jsonschema-form forces downstream projects to use babel-polyfill, and includes duplicated babel helpers.
This removes those duplicate helpers and adds ponyfills for modern globals like Set, Promise and Objects.assign.
This PR also replaces methods like
Array.prototype.includes
andArray.prototype.fill
with ponyfills fromcore-js
If this is related to existing tickets, include links to them as well:
#798
Checklist
npm run cs-format
on my branch to conform my code to prettier coding style