Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enable the script to load in DoneJS SSR #8904

Merged
merged 1 commit into from
Jul 12, 2016

Conversation

marshallswain
Copy link
Contributor

@marshallswain marshallswain commented Jun 6, 2016

Adding this check for script && script.parentNode allows foundation-sites to work with DoneJS's Server Side Rendering.

This fixes an issue where the limited DOM used by done-ssr would prevent the script from running and would fail with the following error:

Error evaluating file:/Users/marshall/Sites/gh-test/public/node_modules/foundation-sites/dist/foundation.js
Cannot read property 'parentNode' of undefined
    at file:/Users/marshall/Sites/gh-test/public/node_modules/foundation-sites/dist/foundation.js:855:13

I don't think I can make a test for this.

Adding this check for `script && script.parentNode` allows foundation-sites to work with DoneJS's Server Side Rendering.
@marshallswain
Copy link
Contributor Author

marshallswain commented Jun 6, 2016

I watched the tests pass (why wouldn't they, it's just a variable check?) and then the phantom install failed.

@abarnwell
Copy link
Contributor

@marshallswain I've had the same problem with my tests. Travis CI has a hiccup now and again.

@andycochran
Copy link
Contributor

@kball You wanna weigh in on this PR and nudge Travis to rebuild if you think it's a good change?

@rafibomb
Copy link
Member

@kball This is over my head but I restarted Travis

@rafibomb rafibomb added this to the 6.2.4 milestone Jul 12, 2016
@kball
Copy link
Contributor

kball commented Jul 12, 2016

This seems fine; I don't know if it will generally help other server-side rendering frameworks, or even if it is sufficient for DoneJS (there may be other assumptions in other parts of the chain) but it's not going to break anything in the client-side environment, so I see no harm in merging.

@kball kball merged commit 25fe644 into foundation:develop Jul 12, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants