Support Server Rendering of `amp` Attribute #6798

Closed
sebmarkbage opened this Issue May 18, 2016 · 16 comments

Comments

Projects
None yet
10 participants
@sebmarkbage
Member

sebmarkbage commented May 18, 2016

Are there other attributes like this that necessitates server rendering?

Most features have corresponding DOM properties that we prefer. There's some case with app cache that I know about as well that only has server-side meaning.

If this is a more generic problem we might need a way to solve this server-side. We don't want global plugins. The client side solution is just refs and manual manipulation so we generally don't need to fix it. It is uncommon enough and doesn't create ecosystem fragmentation/isolation by global plugins (I learned something from MooTools).

@poscar

This comment has been minimized.

Show comment
Hide comment
@poscar

poscar May 19, 2016

@sebmarkbage Thanks for opening this issue! +1

poscar commented May 19, 2016

@sebmarkbage Thanks for opening this issue! +1

@poscar

This comment has been minimized.

Show comment
Hide comment

poscar commented May 19, 2016

AMP spec here

@poscar

This comment has been minimized.

Show comment
Hide comment
@poscar

poscar May 19, 2016

@sebmarkbage It looks like I won't be able to use React server rendering for this. There are other attributes as well, such as amp-boilerplate that need to be included. I tried doing is amp-boilerplate, but the AMP validator then complains about the is attribute...

UPDATE:
Turns out the workaround wasn't working because it wasn't being applied to the React module actually loaded by the dependency of my project. I manage to get the following to work:

const DOMProperty = require('react/lib/ReactInjection').DOMProperty;
DOMProperty.injectDOMPropertyConfig({
  Properties: {
    amp: DOMProperty.MUST_USE_ATTRIBUTE,
    'amp-boilerplate': DOMProperty.MUST_USE_ATTRIBUTE,
    'amp-custom': DOMProperty.MUST_USE_ATTRIBUTE
  },
  isCustomAttribute: (attributeName) => {
    return attributeName.startsWith('amp');
  }
});

poscar commented May 19, 2016

@sebmarkbage It looks like I won't be able to use React server rendering for this. There are other attributes as well, such as amp-boilerplate that need to be included. I tried doing is amp-boilerplate, but the AMP validator then complains about the is attribute...

UPDATE:
Turns out the workaround wasn't working because it wasn't being applied to the React module actually loaded by the dependency of my project. I manage to get the following to work:

const DOMProperty = require('react/lib/ReactInjection').DOMProperty;
DOMProperty.injectDOMPropertyConfig({
  Properties: {
    amp: DOMProperty.MUST_USE_ATTRIBUTE,
    'amp-boilerplate': DOMProperty.MUST_USE_ATTRIBUTE,
    'amp-custom': DOMProperty.MUST_USE_ATTRIBUTE
  },
  isCustomAttribute: (attributeName) => {
    return attributeName.startsWith('amp');
  }
});
@busticated

This comment has been minimized.

Show comment
Hide comment
@busticated

busticated Jul 15, 2016

i'm not 100% on it yet but it would seem webkit-playsinline falls into this category as well with the changes around inline & auto playing videos on iOS 10.

Inline and Auto Video Playback on iOS

When the webkit-playsinline property is specified, Safari on iPhone allows videos to play inline. Videos without the property will commence playback in fullscreen, but users can pinch close on the video to continue playing the video inline.

On iOS, videos without audio tracks or with disabled audio tracks can play automatically when the webpage loads.

source

i'm not 100% on it yet but it would seem webkit-playsinline falls into this category as well with the changes around inline & auto playing videos on iOS 10.

Inline and Auto Video Playback on iOS

When the webkit-playsinline property is specified, Safari on iPhone allows videos to play inline. Videos without the property will commence playback in fullscreen, but users can pinch close on the video to continue playing the video inline.

On iOS, videos without audio tracks or with disabled audio tracks can play automatically when the webpage loads.

source

@dantman

This comment has been minimized.

Show comment
Hide comment
@dantman

dantman Aug 3, 2016

Contributor

@sebmarkbage Would libraries injecting features globally be more acceptable if they use a method that cannot conflict with another library?

We do have Symbols now in JS.

import amp from 'react-amp';

const Foo = () => (
  React.createElement('html', {[amp]: true}/*, ... */)
);

If you're worried about the situation where there are multiple copies of react, we could demand that plugins use a registration function.

import amp, {register as registerAmp} from 'react-amp';
// ...

registerAmp(React);
export default () => ReactDOM. renderToString(<Foo />);

A symbol check will also allow libraries to know if they have already been registered, so registerAmp being called multiple times on the same React or multiple instances of react-amp won't be a problem either.

Contributor

dantman commented Aug 3, 2016

@sebmarkbage Would libraries injecting features globally be more acceptable if they use a method that cannot conflict with another library?

We do have Symbols now in JS.

import amp from 'react-amp';

const Foo = () => (
  React.createElement('html', {[amp]: true}/*, ... */)
);

If you're worried about the situation where there are multiple copies of react, we could demand that plugins use a registration function.

import amp, {register as registerAmp} from 'react-amp';
// ...

registerAmp(React);
export default () => ReactDOM. renderToString(<Foo />);

A symbol check will also allow libraries to know if they have already been registered, so registerAmp being called multiple times on the same React or multiple instances of react-amp won't be a problem either.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Aug 3, 2016

Contributor

@dantman IMHO the first is probably a realistic path (i.e. locally provide configuration rather than register it globally), but I doubt that's the future for this feature at least. I'm not really in the loop on this, so take it with a grain of salt, but it seems the intention for unknown props is to be passed through to the node as-is in the near future. Which should solve the general problem people have.

Contributor

syranide commented Aug 3, 2016

@dantman IMHO the first is probably a realistic path (i.e. locally provide configuration rather than register it globally), but I doubt that's the future for this feature at least. I'm not really in the loop on this, so take it with a grain of salt, but it seems the intention for unknown props is to be passed through to the node as-is in the near future. Which should solve the general problem people have.

@pgoldrbx

This comment has been minimized.

Show comment
Hide comment
@pgoldrbx

pgoldrbx Nov 18, 2016

Adding a note in this thread for the workaround above from @poscar
This workaround requires an update as of the 15.4.0 release to account for the formal react-dom split.

const DOMProperty = require('react-dom/lib/ReactInjection').DOMProperty;

pgoldrbx commented Nov 18, 2016

Adding a note in this thread for the workaround above from @poscar
This workaround requires an update as of the 15.4.0 release to account for the formal react-dom split.

const DOMProperty = require('react-dom/lib/ReactInjection').DOMProperty;
@Ariel-Rodriguez

This comment has been minimized.

Show comment
Hide comment
@Ariel-Rodriguez

Ariel-Rodriguez Nov 27, 2016

@pgoldrbx thanks for the advice, it was a great help.
https://github.com/Ariel-Rodriguez/react-amp-template/blob/master/src/core/index.js#L4

Despite i am almost beginner with node / SSR , i wrote this small library which facilitated very well to some partners from my work. I'd be very proud if It helped to anyone else.

Ariel-Rodriguez commented Nov 27, 2016

@pgoldrbx thanks for the advice, it was a great help.
https://github.com/Ariel-Rodriguez/react-amp-template/blob/master/src/core/index.js#L4

Despite i am almost beginner with node / SSR , i wrote this small library which facilitated very well to some partners from my work. I'd be very proud if It helped to anyone else.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 4, 2017

Member

Note that the code in earlier comments won’t work in React 16, but it’s also not needed because React 16 passes custom attributes through, including amp, as long as the values are strings or numbers. Booleans need to be written explicitly as strings.

I’ll close this issue but if there’s more that’s necessary specifically for AMP please file a new one with details.

Member

gaearon commented Oct 4, 2017

Note that the code in earlier comments won’t work in React 16, but it’s also not needed because React 16 passes custom attributes through, including amp, as long as the values are strings or numbers. Booleans need to be written explicitly as strings.

I’ll close this issue but if there’s more that’s necessary specifically for AMP please file a new one with details.

@gaearon gaearon closed this Oct 4, 2017

@misscs

This comment has been minimized.

Show comment
Hide comment
@misscs

misscs Dec 7, 2017

Note that the code in earlier comments won’t work in React 16, but it’s also not needed because React 16 passes custom attributes through, including amp, as long as the values are strings or numbers.

This works for amp attributes that take values, but many attributes don't, and cannot to correctly pass amp validation.

Example: HTML element needs amp attribute: <html amp>. React 16 strips the attribute unless a string is passed. <html amp="true"> is not AMP valid.

@gaearon Should I open raise this in a new issue?

misscs commented Dec 7, 2017

Note that the code in earlier comments won’t work in React 16, but it’s also not needed because React 16 passes custom attributes through, including amp, as long as the values are strings or numbers.

This works for amp attributes that take values, but many attributes don't, and cannot to correctly pass amp validation.

Example: HTML element needs amp attribute: <html amp>. React 16 strips the attribute unless a string is passed. <html amp="true"> is not AMP valid.

@gaearon Should I open raise this in a new issue?

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Dec 7, 2017

Contributor

@misscs <html amp=""> should work though (<html amp={true}> should work too IIRC). AFAIK unless it's been changed React never renders attributes as valueless (for XML-compatibility), but valueless and empty string should be the same unless amp is unreasonably strict in its validation.

Contributor

syranide commented Dec 7, 2017

@misscs <html amp=""> should work though (<html amp={true}> should work too IIRC). AFAIK unless it's been changed React never renders attributes as valueless (for XML-compatibility), but valueless and empty string should be the same unless amp is unreasonably strict in its validation.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Dec 7, 2017

Contributor

I believe <html amp="amp"> is the unabbreviated form that's valid in HTML?

Contributor

ljharb commented Dec 7, 2017

I believe <html amp="amp"> is the unabbreviated form that's valid in HTML?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Dec 7, 2017

Member

Yes, both amp="" and amp="amp" should work.

AFAIK unless it's been changed React never renders attributes as valueless (for XML-compatibility)

Oops, we actually did change this in #11708 (cc @aweary), but haven't released yet. Was that important for some use case? It was not clear to me.

Member

gaearon commented Dec 7, 2017

Yes, both amp="" and amp="amp" should work.

AFAIK unless it's been changed React never renders attributes as valueless (for XML-compatibility)

Oops, we actually did change this in #11708 (cc @aweary), but haven't released yet. Was that important for some use case? It was not clear to me.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Dec 7, 2017

Contributor

@gaearon Someone complained about XML-compatibility way back and "you" changed it to never render valueless attributes. With the new renderer I guess it's less of an issue since it should only affect SSR.

Contributor

syranide commented Dec 7, 2017

@gaearon Someone complained about XML-compatibility way back and "you" changed it to never render valueless attributes. With the new renderer I guess it's less of an issue since it should only affect SSR.

@syranide

This comment has been minimized.

Show comment
Hide comment
Contributor

syranide commented Dec 7, 2017

@gaearon #2842

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Dec 7, 2017

Member

Oh okay. Thanks for digging. I guess I'd prefer to drop the support for this if it's not popular, but that's definitely a breaking change and we can't do it now.

Member

gaearon commented Dec 7, 2017

Oh okay. Thanks for digging. I guess I'd prefer to drop the support for this if it's not popular, but that's definitely a breaking change and we can't do it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment