-
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
📦 React 15 support #2426
📦 React 15 support #2426
Conversation
well it's really not that bad for us to be React 15 compatible! nice stuff |
packages/core/package.json
Outdated
@@ -45,8 +45,8 @@ | |||
"tslib": "^1.9.0" | |||
}, | |||
"peerDependencies": { | |||
"react": "^16.2.0", | |||
"react-dom": "^16.0.0", | |||
"react": ">=15.3.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.
Worth setting an upper bound here?
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.
it's this or 15 || 16
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.
Comments and questions
|
||
export interface IPortalProps extends IProps, HTMLDivProps { | ||
const isReact15 = typeof ReactDOM.createPortal === "undefined"; |
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.
rename this to a more accurate name like canCreatePortal
or reactApiHasCreatePortal
or whatever
packages/test-commons/bootstrap.js
Outdated
@@ -2,9 +2,13 @@ | |||
* Copyright 2017 Palantir Technologies, Inc. All rights reserved. | |||
*/ | |||
|
|||
import "./polyfill"; | |||
require("./polyfill"); |
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.
why is it necessary to switch to commonjs for this test code? shouldn't the node version that runs it support es6 modules?
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.
necessary because isotest uses this now and that suite explicitly runs in a node environment (for server-side rendering). added code comment.
"private": true, | ||
"dependencies": { | ||
"enzyme-adapter-react-15": "^1.0.5", | ||
"react": "^15.6.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.
since react 16 is already released, should we just pin this to the final 15.x version?
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.
there could be future patches? this is 15.6.x
Merge branch 'develop' of github.com:palantir/blueprint into gg/react15Preview: documentation | landing | table |
revert require() syntax where possiblePreview: documentation | landing | table |
peer dependency `^15.3.0 || 16`Preview: documentation | landing | table |
revert to commonjs syntax with a commentPreview: documentation | landing | table |
@@ -30,4 +30,18 @@ application. | |||
`Portal` supports the following options on its [React context](https://facebook.github.io/react/docs/context.html). | |||
To use them, supply a child context to a subtree that contains the Portals you want to customize. | |||
|
|||
<div class="@ns-callout @ns-intent-primary @ns-icon-info-sign"> | |||
Blueprint uses the React 15-compatible `getChildContext()` API instead of the newer React 16.3 `React.createContext()` API. |
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.
What if you want to use React.createContext()
or some other new React feature in the future? Supporting both versions might seem like a few hundred LOC right now, but it could end up being pretty limiting.
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.
i agree, it is quite limiting, but it's the quickest path to adoption.
shim/ponyfill packages are available, such as create-react-context, which is actually used in the latest version of react-popper to support multiple versions. so we could go down that route, like we did with pure-render-decorator.
semantic merge circle configPreview: documentation | landing | table |
circle cachePreview: documentation | landing | table |
seems to bork karma-junit-reorter karma-runner/karma-junit-reporter#116
remove console color from enzyme messagePreview: documentation | landing | table |
Changes proposed in this pull request:
core
peer dependency"react" >=15.3.0
(forPureComponent
)Portal
now degrades toReactDOM.unstable_renderSubtreeIntoContainer
ifReactDOM.createPortal
is not availableButton