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

Feature request: Support server-side rendering of non-standard DOM attribute names (eg. AMP's [prop]="value") #10064

Closed
cole-sanderson opened this issue Jun 28, 2017 · 10 comments
Assignees

Comments

@cole-sanderson
Copy link

cole-sanderson commented Jun 28, 2017

Do you want to request a feature or report a bug?
Request a feature

I am working on project to build AMP page with React Server Side Rendering. I am having an issue to add custom attribute to built-in AMP element. In order to be able to use amp-bind we need to be able to output “bindings”, which are special attributes of the form [attribute], eg. [slide]="selectedSlide".

<amp-carousel 
    layout={layout}
    height={height}
    width={width}
    [slide]={slide}
>
        ...
</amp-carousel>

Here is AMP carousel example that work with amp-bind.

What is the current behavior?

  • Parsing error: Unexpected token [ (Fatal)

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

  • All

For more information, you can read all the discussion in this PR.

@aweary
Copy link
Contributor

aweary commented Jul 11, 2017

Thanks for the request @cole-sanderson! Once #7311 is merged this should be possible, with a caveat. The error you're getting:

- Parsing error: Unexpected token [ (Fatal)

Is because [slide]={slide} is not valid JSX, so parsing fails. I think it's unlikely that the JSX spec would be updated to support this syntax unless it would provide some benefit for a broader scope of issues/features.

What that means is that you'll just need to use the regular function call syntax for elements that require these AMP attributes.

React.createElement('amp-carousel', {
  layout: layout,
  height: height,
  width: width,
  '[slide]': slide
}

After #7311 is merged the server renderer will not blacklist the [slide] property and the markup should be rendered correctly.

Keep in mind this is only for server-side rendering. [slide] is not a valid DOM attribute name, so if you're intention is to hydrate this server-rendered markup on an AMP page with React it won't work. Though I assume AMP will manage the content on the client, so that might not even be a concern.

@aweary
Copy link
Contributor

aweary commented Oct 4, 2017

The solution outlined above should now work with React 16.

@aweary aweary closed this as completed Oct 4, 2017
@cole-sanderson
Copy link
Author

Awesome, thank you @aweary! 😀

@kschmidtdev
Copy link

@aweary according to https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html#changes-in-detail there isn't support for custom DOM elements (amp-carousel) with custom attributes (slide). I've tried the createElement call in react server-side rendering with no success. Can you confirm?

@aweary
Copy link
Contributor

aweary commented Feb 7, 2018

hey @kschmidtdev, you are correct. The server renderer still restricts attributes using isAttributeNameSafe. I don't remember why I thought this had changed in React 16, but neither #7311 nor it's successor #10385 change this behavior.

I'll re-open this. I'm not sure what the solution here would be; it's unfortunate that AMP uses non-standard attribute names. If server rendering AMP components is a common use case then we could potentially fork isAttributeNameSafe to a version that allows attributes with leading/trailing brackets on custom elements.

@gaearon what do you think?

@aweary aweary reopened this Feb 7, 2018
@svdoever
Copy link

Would really appreciate a solution for this. Doing server-side AMP rendering as well using react. Will never dehydrate on client.

@dfrankland
Copy link

Just to add to the confusion, you can still add attributes with square brackets to non-custom components:

Example:

  • JS

    const customProps = {
      "[custom-property]": "Hello, world!"
    };
  • JSX

    <div>
      <div {...customProps}>Test</div>
      <custom-component {...customProps}>Test</custom-component>
    </div>
  • Resulting HTML

    <div data-reactroot="">
      <div [custom-property]="Hello, world!">Test</div>
      <custom-component>Test</custom-component>
    </div>

This throws a warning only for the <custom-component />.

@dfrankland
Copy link

I've opened #12568 to add the ability to, at the very least, server-render non-standard components with non-standard attributes (like AMP).

@westonruter
Copy link

For amp-bind there is now an XML-compatible syntax, so you can do data-amp-bind-foo="bar" as an alternative to [foo]="bar". See ampproject/amphtml#11115 and ampproject/amphtml#15408. So it doesn't seem that support for the amp-bind bracketed attribute syntax would be required in React.

@aweary
Copy link
Contributor

aweary commented Sep 6, 2018

Great! In that case I don't think it's worth the effort to support the XML-incompatible syntax.

@aweary aweary closed this as completed Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants