-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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
Create react-addons-dom-factories package #8356
Create react-addons-dom-factories package #8356
Conversation
8bbd4e3
to
f319823
Compare
@@ -891,7 +894,6 @@ src/renderers/dom/stack/client/__tests__/ReactDOM-test.js | |||
* should allow children to be passed as an argument | |||
* should overwrite props.children with children argument | |||
* should purge the DOM cache when removing nodes | |||
* allow React.DOM factories to be called without warnings |
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.
Did some furniture moving of the factory tests, which failed CI. So I updated the test recording using:
scripts/fiber/record-tests --track-facts
Did I do this correctly?
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.
You shouldn't need --track-facts
.
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.
Yeah, I figured that out later, when I hit a permissions issue not having write access to the repo. Everything else worked perfectly.
We can't do this in a single pass. As a first step we should create the new package and add a deprecation warning when you access |
Ah. Makes sense. I'll bring it back. |
f319823
to
618ed03
Compare
Sort of tangental, but this was my first pseudo-production use of a Proxy. What a cool API. It's a bummer |
add4fc2
to
da03cba
Compare
I've upstreamed it with master, fixed some tests, and re-recorded the fiber output. I don't want this PR to be too much of a thorn in your side. I'll periodically keep it up to date with master so it's not difficult to update whenever it makes sense to evaluate it. |
expect(a.type).toBe('a'); | ||
expect(p.type).toBe('p'); | ||
|
||
expectDev(console.error.calls.count()).toBe(1); |
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.
Can you add an assertion on the content of the warning? It's not a big deal, but I like to make sure the tests are checking for the correct warning, not just any warning.
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.
Sure can.
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.
Done, though it feels a bit cumbersome. Do you know of a better way to make this check?
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 think the way you did it is how it is done throughout the React tests.
Ack! those double quotes get me every time. Fixed. |
f2e8170
to
1612436
Compare
1612436
to
8f6d3e8
Compare
8f6d3e8
to
2458057
Compare
Conflicts resolved. Anything else I can do here to help move this along? |
docs/docs/addons-dom-factories.md
Outdated
title: DOM Factories | ||
permalink: docs/dom-factories.html | ||
layout: docs | ||
category: Add-Ons |
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 removed the 'Add-Ons' section of the docs from the main navigation when we deprecated most of them in v15.5. (#9359) I think it's worth having this documentation - maybe we could put it in the README
for the new separate module?
src/isomorphic/React.js
Outdated
// accesses to the React.DOM namespace and alert users to switch to | ||
// the `react-addons-dom-factories` package. | ||
if (typeof Proxy === 'function') { | ||
var hasWarnedOfFactories = false; |
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 really like this use of a Proxy
. I'm wondering though about the fact that, according to MDN, Proxy is not yet supported in IE. To also show this warning in IE, we may want to switch and add the warning in the copy of ReactDOMFactories
that remains in React. We could do that in a follow-up PR though.
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.
Maybe we should instead use Object.defineProperty
to define getters for each method, so we don't have to rely on Proxy
at all and split up the logic for these warnings.
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.
Using Object.defineProperty
could be handy.
With Object.defineProperty
it might look like this:
if (__DEV__) {
// React.DOM factories are deprecated. ...(rest of comment)
var hasWarnedOfFactories = false;
var DOMFactoryMethodNameList = ['a', 'abbr', 'address', ... 'text', 'tspan'];
DOMFactoryMethodNameList.forEach((method) => {
React.DOM = Object.defineProperty(React.DOM, method, {
get: function() {
if (!hasWarnedOfFactories) {
warning(
false,
'Accessing factories like React.DOM.%s has been deprecated ' +
'and will be removed in the future. Use the ' +
'react-addons-dom-factories package instead.',
method
);
hasWarnedOfFactories = true;
}
return React.DOM[method];
},
});
});
}
Unless there is a way to define a generic 'get' for all object attributes with Object.defineProperty
?
If not then it seems just as easy to do without the Object.defineProperty
call I think, something like this;
if (__DEV__) {
// React.DOM factories are deprecated. ...(rest of comment)
var hasWarnedOfFactories = false;
var DOMFactoryMethodNameList = ['a', 'abbr', 'address', ... 'text', 'tspan'];
DOMFactoryMethodNameList.forEach((method) => {
var wrappedMethod = React.DOM[method];
React.DOM[method] = function() {
if (!hasWarnedOfFactories) {
warning(
false,
'Accessing factories like React.DOM.%s has been deprecated ' +
'and will be removed in the future. Use the ' +
'react-addons-dom-factories package instead.',
method
);
hasWarnedOfFactories = true;
}
return wrappedMethod.apply(null, Array.prototype.slice.call(arguments));
});
});
}
Both of these instantiate an extra wrapper function for every factory method, which is annoying. I figure a follow-up PR, either by me or someone else, can come up with a good approach.
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 could simply it further by using a for ... in
loop over ReactDOMFactories
!
if (__DEV__) {
var warnedForFactories = false;
for (var factory in ReactDOMFactories) {
React.DOM[factory] = function(...args) {
if (!warnedForFactories) {
warning(
false,
'Accessing factories like React.DOM.%s has been deprecated ' +
'and will be removed in the future. Use the ' +
'react-addons-dom-factories package instead.',
factory
)
warnedForFactories = true;
}
return ReactDOMFactories[factory](...args);
}
}
}
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 like this, 👍
Thanks for your patience on this PR @nhunzaker! We plan on merging it both into master, which will be v16.0, and the v15.6-dev branch. Unfortunately the build process on master has changed quite a bit. I think the simplest way is this:
Hope that makes sense. :) |
@flarnie Sorry for the late response. All of this makes a ton of sense. I should be able to resolve the feedback later today. |
Since we are deprecating the 'React.DOM.*' factories,[1] we want to provide a codemod so that it's easier for folks to upgrade their code. This will include an option to use 'React.createFactory' instead of 'React.createElement' in case there is a use case where that is preferred. There is one use of `React.DOM.*` that I have seen which is not covered here - sometimes it has mistakenly been used for Flow typing. In the cases I have found it is not proper syntax and doesn't seem like something we should cover with this codemod. [1]: facebook/react#9398 and facebook/react#8356
Since we are deprecating the 'React.DOM.*' factories,[1] we want to provide a codemod so that it's easier for folks to upgrade their code. This will include an option to use 'React.createFactory' instead of 'React.createElement' in case there is a use case where that is preferred. There is one use of `React.DOM.*` that I have seen which is not covered here - sometimes it has mistakenly been used for Flow typing. In the cases I have found it is not proper syntax and doesn't seem like something we should cover with this codemod. [1]: facebook/react#9398 and facebook/react#8356
Since we are deprecating the 'React.DOM.*' factories,[1] we want to provide a codemod so that it's easier for folks to upgrade their code. This will include an option to use 'React.createFactory' instead of 'React.createElement' in case there is a use case where that is preferred. There is one use of `React.DOM.*` that I have seen which is not covered here - sometimes it has mistakenly been used for Flow typing. In the cases I have found it is not proper syntax and doesn't seem like something we should cover with this codemod. [1]: facebook/react#9398 and facebook/react#8356
Since we are deprecating the 'React.DOM.*' factories,[1] we want to provide a codemod so that it's easier for folks to upgrade their code. This will include an option to use 'React.createFactory' instead of 'React.createElement' in case there is a use case where that is preferred. There is one use of `React.DOM.*` that I have seen which is not covered here - sometimes it has mistakenly been used for Flow typing. In the cases I have found it is not proper syntax and doesn't seem like something we should cover with this codemod. [1]: facebook/react#9398 and facebook/react#8356
Moves React DOM factories out of React, makes new
react-addons-dom-factories
package. Adds basic documentation for new addon.I hope this is most of the legwork for #6169.
Sort of tangental, but this reduces React down to 5.3kb gzipped. Pretty neat :)