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

Don't assume window is defined when polyfilling Promise #1254

Closed
wants to merge 1 commit into from

Conversation

pugnascotia
Copy link
Contributor

I know server-rendering isn't supported, but I was tinkering with loading the production bundle in Nashorn, Java 8's JavaScript engine, and found that the Promise polyfill assumes that window exists. This PR changes the polyfill to attempt to use window first, and global secondly.

For the interested reader, global isn't defined in Nashorn either, but it can be easily defined in order to bring it in-line with Node.

Testing steps:

npm run create-react-app mytestapp
cd mytestapp
npm start
# App starts OK and renders in the browser

npm run build
# App builds successfully.

# Also need to change generated src/index.js to only mount React if document is defined
vim src/index.js
# Start Nashorn interpreter
jjs
> var global = {}
> load('build/static/js/main.26610bf4.js')
# File load without throwing an exception

@gaearon
Copy link
Contributor

gaearon commented Dec 12, 2016

loading the production bundle in Nashorn

Can you describe the use case?
I’m worried that even if we take this, we might break it again in the future since we don‘t support SSR.

@pugnascotia
Copy link
Contributor Author

The use case? That would be...SSR :-) It was the only library-side change I had to make to get it to load in Nashorn, so I thought it was benign enough, but I do take the point about it not being supported.

@gaearon
Copy link
Contributor

gaearon commented Dec 12, 2016

How do you know which file to execute? Are you using the generated manifest?

@pugnascotia
Copy link
Contributor Author

I haven't gotten that far yet but yes, that would be the obvious thing to do.

@gaearon
Copy link
Contributor

gaearon commented Dec 12, 2016

I think I won’t take it as is for the reasons above.
But I’m open to review it as a part of something like #1100.

I’d like to approach this as a holistic feature so we understand how to keep it working and extent to which we support this, and don’t break it in the future.

Thanks!

@gaearon gaearon closed this Dec 12, 2016
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants