Skip to content
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

Fixing the instantiation of customized builtin elements (related to custom elements) #9313

Merged
merged 5 commits into from Apr 19, 2017

Conversation

joeldenning
Copy link
Contributor

The custom element spec, combined with the document.createElement spec explain that in order to create a DOM element as a customized built-in element, you should call document.createElement with an object as the second parameter. Customized builtin elements are a type of custom element that allow you to extend native DOM elements. However, React currently provides the second parameter to document.createElement as a string instead of an object. As far as I can tell, that is just a bug.

I am not sure of the full history of the custom elements spec, and so it might be possible that the spec used to say the second argument to createElement should be a string. I'm also not sure if anybody is depending on the behavior being like it is right now, or if this change would be considered a breaking change or not for React. My thoughts are that it is a bug fix, but I may be lacking some context here.

@@ -552,7 +552,7 @@ ReactDOMComponent.Mixin = {
div.innerHTML = `<${type}></${type}>`;
el = div.removeChild(div.firstChild);
} else if (props.is) {
el = ownerDocument.createElement(type, props.is);
el = ownerDocument.createElement(type, {is: props.is});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://dom.spec.whatwg.org/#dom-document-createelement.

The is prop indicates that the react element should be a customized builtin element.

@@ -1045,7 +1045,7 @@ describe('ReactDOMComponent', () => {
ReactDOM.render(<div is="custom-div" />, container);
expect(document.createElement).toHaveBeenCalledWith(
'div',
'custom-div',
{is: 'custom-div'},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document.createElement should not have a second argument of a string, but rather an options object

@joeldenning
Copy link
Contributor Author

Updated this to fix the build -- I had forgotten to npm run prettier before creating this pr.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Fiber tests are failing (that’s a new engine that’s hidden behind a flag).

There is equivalent code in ReactDOMFiberComponent.js, could you please fix it too and then run scripts/fiber/record-tests?

@joeldenning
Copy link
Contributor Author

joeldenning commented Apr 18, 2017

@gaearon thanks for the help -- I have updated ReactDOMFiberComponent as suggested, but the fiber tests are failing for me locally. The same tests fail for me whether I'm on the master branch or on my branch, so I don't think it's actually related to my code changes. And it changes the tests-failing and tests-passing files, even on the master branch. So what I have done is push my change to ReactDOMFiberComponent without also adding my changes to scripts/fiber/tests-failing.txt and scripts/fiber/tests-passing.txt. I am hoping that maybe the CI environment will be better configured or something so that the tests pass there even though they failed for me locally.

The offending tests are:
src/isomorphic/classic/types/tests/ReactPropTypes-test.js

  • should warn if called manually in development
  • should warn if called manually in development
  • should warn if called manually in development

Maybe related to react@15.5, since it's related to PropTypes??? I am not sure if it's just my local env that is causing issues or if maybe the tests-failing.txt file has not been updated since react@15.5?? Would appreciate any help you could offer.

@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2017

I am not sure of the full history of the custom elements spec, and so it might be possible that the spec used to say the second argument to createElement should be a string

Pretty sure this is the case. MDN:

For backwards compatibility with previous versions of the Custom Elements specification, some browsers will allow you to pass a string here instead of an object, where the string's value is the custom element's tag name.

The feature itself was originally added in #6570, and I don’t think it was extensively tested or updated to match the latest spec.

For safety I’ll merge this to master but won’t backport to 15.x. Thanks!

@gaearon gaearon merged commit 233195c into facebook:master Apr 19, 2017
@gaearon gaearon added this to the 16.0 milestone Apr 19, 2017
@joeldenning joeldenning deleted the customized-builtin-elements branch April 19, 2017 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants