Skip to content

[SSR] Removed call of requestAnimationFrame outside of function#975

Closed
fezproof wants to merge 6 commits into
adobe:mainfrom
fezproof:main
Closed

[SSR] Removed call of requestAnimationFrame outside of function#975
fezproof wants to merge 6 commits into
adobe:mainfrom
fezproof:main

Conversation

@fezproof
Copy link
Copy Markdown

@fezproof fezproof commented Aug 17, 2020

This allows the base library of @adobe/react-spectrum to used in Gatsby

Closes #842

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  • Create a basic hello-world gatsby app
  • Add this snippet to your app
<Provider theme={defaultTheme}>
  <Flex direction="column" width="size-2000" gap="size-100">
      <View>this is a thing</View>
      <View>this is a thing</View>
      <View>this is a thing</View>
      <View>this is a thing</View>
    </Flex>
</Provider>
  • Build!

Without this change, the gatsby build will try to run rAF as it is outside of a actually hook function that would normally be called in the render loop. This caused the error in #835. With this simple change, functionality is essentially the same, but it will now compile in a Gatsby enviroment.
The only real loss is that the check will happen every animation frame, but it is a semi-cheap comparison so it shouldn't effect any actual animations.
See https://developers.google.com/web/updates/2012/05/requestAnimationFrame-API-now-with-sub-millisecond-precision#feature_detection to show the change shouldn't make any reasonable difference to the code.

This allows the base library of @adobe/react-spectrum to used in Gatsby
Fixes #842
@fezproof fezproof changed the title Removed call of requestAnimationFrame outside of function [SSR] Removed call of requestAnimationFrame outside of function Aug 19, 2020
devongovett
devongovett previously approved these changes Aug 19, 2020
Copy link
Copy Markdown
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Thank you!

// if we're using a high res timer, make sure timestamp is not the old epoch-based value.
// http://updates.html5rocks.com/2012/05/requestAnimationFrame-API-now-with-sub-millisecond-precision
if (fixTs) {
if (t < 1e12) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, I think this is the opposite of what we need? If the time is less than 1e12 then it's a high res timer already. I think we want t > 1e12 here instead?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And also we might need to check the resolution of performance.now() as the above check does. e.g. in safari performance.now() is less than 1e12 (I think after the spectre/meltdown attacks).

Maybe we should just put the check above in an if statement to check if requestAnimationFrame even exists?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I will experiment with this and get back to you!

Copy link
Copy Markdown
Author

@fezproof fezproof Aug 20, 2020

Choose a reason for hiding this comment

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

@devongovett Finished testing, doing the compare inside the animation seems to be the only option :(
I have updated the PR + added some consts to ease my conscience 😁

Copy link
Copy Markdown
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Thanks again

@devongovett
Copy link
Copy Markdown
Member

Merged manually

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error building gatsby.js, requestAnimationFrame is not defined

2 participants