-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Initialize compat options when relevant functions are called #2156
base: main
Are you sure you want to change the base?
Changes from all commits
cfdd3b7
0276782
82137b4
dd3157d
5db377c
3b5a27c
3d34c5f
3ea546e
21d4a07
e8f1ba9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { Component as PreactComponent } from '../../src/index'; | ||
// import { installReactCompat, isReactCompatInstalled } from './render'; | ||
|
||
// export class Component extends PreactComponent { | ||
// constructor(props, context) { | ||
// super(props, context); | ||
// | ||
// if (!isReactCompatInstalled) { | ||
// installReactCompat(); | ||
// } | ||
// } | ||
// } | ||
|
||
export function Component() { | ||
PreactComponent.apply(this, arguments); | ||
|
||
// if (!isReactCompatInstalled) { | ||
// installReactCompat(); | ||
// } | ||
|
||
// this.setState = PreactComponent.prototype.setState; | ||
// this.forceUpdate = PreactComponent.prototype.forceUpdate; | ||
// this.render = PreactComponent.prototype.render; | ||
} | ||
|
||
// Component.prototype.setState = PreactComponent.prototype.setState; | ||
// Component.prototype.forceUpdate = PreactComponent.prototype.forceUpdate; | ||
// Component.prototype.render = PreactComponent.prototype.render; | ||
|
||
// Component.prototype = Object.create(PreactComponent); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,36 @@ | ||
import { Component } from 'preact'; | ||
import { Component } from './Component'; | ||
import { shallowDiffers } from './util'; | ||
|
||
/** | ||
* Component class with a predefined `shouldComponentUpdate` implementation | ||
*/ | ||
export class PureComponent extends Component { | ||
constructor(props) { | ||
super(props); | ||
// Some third-party libraries check if this property is present | ||
this.isPureReactComponent = true; | ||
} | ||
// export class PureComponent extends Component { | ||
// constructor(props) { | ||
// super(props); | ||
// // Some third-party libraries check if this property is present | ||
// this.isPureReactComponent = true; | ||
// } | ||
// | ||
// shouldComponentUpdate(props, state) { | ||
// return ( | ||
// shallowDiffers(this.props, props) || shallowDiffers(this.state, state) | ||
// ); | ||
// } | ||
// } | ||
|
||
shouldComponentUpdate(props, state) { | ||
return ( | ||
shallowDiffers(this.props, props) || shallowDiffers(this.state, state) | ||
); | ||
} | ||
export function PureComponent(props, context) { | ||
this.props = props; | ||
this.context = context; | ||
|
||
// Some third-party libraries check if this property is present | ||
// this.isReactComponent = {}; | ||
// this.isPureReactComponent = true; | ||
} | ||
|
||
PureComponent.prototype.isReactComponent = {}; | ||
PureComponent.prototype.isPureReactComponent = true; | ||
PureComponent.prototype.setState = Component.prototype.setState; | ||
PureComponent.prototype.forceUpdate = Component.prototype.forceUpdate; | ||
PureComponent.prototype.shouldComponentUpdate = function(props, state) { | ||
return shallowDiffers(this.props, props) || shallowDiffers(this.state, state); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
# Tree shaking in compat TODO | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll remove this file before checking in. Just left it here for discussion |
||
|
||
## Classes | ||
|
||
### The problem | ||
|
||
In order to enable tree-shaking, we need to transform our classes into a tree-shakeable form. | ||
|
||
If a class doesn't use inheritance, then the standard prototype assignments should be tree-shakeable. However classes that inherit (or manually inherit using `Suspense.prototype = new Component()`) are not tree-shakeable by default. BabelJS compiles classes into function closures with a `/*#__PURE__*/` comment informing tree-shakers that this closure can be safely removed if unused. | ||
|
||
However for Preact, since we bundle our code output, rollup sees these classes as used (because they are exported) and terser minifies the output and removes the comments. This means our classes don't get compiled away if a consumer of Preact doesn't use them because of the lack of a `/*#__PURE__*/` comment. | ||
|
||
### Possible solutions | ||
|
||
One option is to modify terser to add a new option to keep `/*#__PURE__*/` comments in source (probably pretty difficult...). | ||
|
||
Another idea would be to manually code the classes to be tree-shakeable and not use inheritance. | ||
|
||
Some code I tried out: | ||
|
||
```js | ||
// portals.js | ||
var ContextProvider = function ContextProvider() {}; | ||
ContextProvider.prototype.getChildContext = function getChildContext() { | ||
return this.props.context; | ||
}; | ||
ContextProvider.prototype.render = function render(props) { | ||
return props.children; | ||
}; | ||
Comment on lines
+23
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defining ContextProvider like this saves us 6 bytes |
||
``` | ||
|
||
```js | ||
// PureComponent.js | ||
// DOESN'T WORK CUZ CONSUMERS NEED TO BE ABLE TO CALL SETSTATE | ||
export function PureComponent() { | ||
this.isPureReactComponent = true; | ||
} | ||
PureComponent.prototype.shouldComponentUpdate = function(props, state) { | ||
return shallowDiffers(this.props, props) || shallowDiffers(this.state, state); | ||
}; | ||
``` | ||
|
||
```js | ||
// suspense.js | ||
|
||
// Comment out the following line to remove a side-effect | ||
// Suspense.prototype = new Component(); | ||
``` | ||
|
||
```js | ||
// suspense-list.js | ||
|
||
// Comment out the following line to remove a side-effect | ||
// SuspenseList.prototype = new Component(); | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,5 @@ | ||
import { options } from 'preact'; | ||
import { assign } from './util'; | ||
|
||
let oldVNodeHook = options.vnode; | ||
options.vnode = vnode => { | ||
if (vnode.type && vnode.type._forwarded && vnode.ref) { | ||
vnode.props.ref = vnode.ref; | ||
vnode.ref = null; | ||
} | ||
|
||
if (oldVNodeHook) oldVNodeHook(vnode); | ||
}; | ||
Comment on lines
-5
to
-12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving this code from compat to core shaves 47 bytes out of compat and only adds 6 bytes to core so I thought it was worth it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And if we didn't install react compat in Component's constructor this entire PR would be free for compat - no bytes added lol. So close! |
||
|
||
/** | ||
* Pass ref down to a child. This is mainly used in libraries with HOCs that | ||
* wrap components. Using `forwardRef` there is an easy way to get a reference | ||
|
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.
a thought: could we use ES5 inheritance 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.
Ah, nevermind, just saw your explanation.