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.

Sign up for GitHub

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

Jump to bottom

[fixed] improvements on setAppElement... #441

Merged
merged 1 commit into from
Jun 25, 2017
Merged

[fixed] improvements on setAppElement... #441

merged 1 commit into from
Jun 25, 2017

Conversation

diasbruno
Copy link
Collaborator

@diasbruno diasbruno commented Jun 25, 2017 •
edited
Loading

There was some pitfalls on how setAppElement works.

If your <script /> was in <head />, there was a change that it tries
to use document.body that is not yet ready.

Another one was using a selector string that does not find any
elements, causing it to try to perform all calls on null. In this case,
we are going to throw an exception.

This patch can also help if you want to do server-side rendering,
but this was not tested and, perhaps, it's better to use this function
correctly.

Fixes #133.

Changes proposed:

  • None

Upgrade Path (for changed or removed APIs):

  • None

Acceptance Checklist:

  • All commits have been squashed to one.
  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.591% when pulling 08f7fbc on diasbruno:fix/set-app-element-behavior into f5d95e2 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.591% when pulling 08f7fbc on diasbruno:fix/set-app-element-behavior into f5d95e2 on reactjs:master.

@diasbruno diasbruno changed the title improvements on setAppElement... [fixed] improvements on setAppElement... Jun 25, 2017
@diasbruno diasbruno force-pushed the fix/set-app-element-behavior branch from 08f7fbc to 6713e33 Compare June 25, 2017 05:35
@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 85.552% when pulling 6713e33 on diasbruno:fix/set-app-element-behavior into f5d95e2 on reactjs:master.

@diasbruno diasbruno force-pushed the fix/set-app-element-behavior branch from 6713e33 to 6ecb2f7 Compare June 25, 2017 16:18
@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 6ecb2f7 on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

@diasbruno diasbruno force-pushed the fix/set-app-element-behavior branch 2 times, most recently from c61501d to 3a99b7c Compare June 25, 2017 20:09
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 3a99b7c on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 3a99b7c on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

@diasbruno diasbruno force-pushed the fix/set-app-element-behavior branch 2 times, most recently from 0ee6c38 to ae1ea8c Compare June 25, 2017 20:12
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 3a99b7c on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 3a99b7c on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

@diasbruno
[fixed] improvements on setAppElement...
52cadf0 
There was some pitfalls on how `setAppElement` work.

If your <script /> was in <head />, there was a change that it tries
to use `document.body` that is not yet ready.

Another one was using an selector string that does not find any
elements, causing it to try to perform all call on `null`.

This patch can also help if you want to do server-side rendering,
but this was not tested and, perhaps, it's better to use this function
correctly.
@diasbruno diasbruno force-pushed the fix/set-app-element-behavior branch from ae1ea8c to 52cadf0 Compare June 25, 2017 20:13
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 52cadf0 on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 52cadf0 on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 52cadf0 on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 52cadf0 on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 85.511% when pulling 52cadf0 on diasbruno:fix/set-app-element-behavior into 5641f40 on reactjs:master.

@diasbruno diasbruno mentioned this pull request Jun 25, 2017
Merged
5 tasks
@diasbruno diasbruno merged commit f8edc2b into reactjs:master Jun 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@diasbruno @coveralls