You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by benkeen <gi...@git.apache.org> on 2015/10/02 21:17:53 UTC
[GitHub] couchdb-fauxton pull request: Add DOM check before mounting/unmoun...
GitHub user benkeen opened a pull request:
https://github.com/apache/couchdb-fauxton/pull/547
Add DOM check before mounting/unmounting React components
The recent API Bar refactor unveiled a rather neat problem that
showed up in the Dashboard only. The Replication page failed to
load and threw a React "Invariant Violation" error which we've
seen show up from time to time but couldn't yet reproduce. What
was happening was that when attempting to mount the component,
the DOM element just didn't exist. The same problem
could occur when unmounting.
Now to go down the rabbithole! This may get boring, be warned...
The reason it occurred in the dash only is because some page
layouts don't contain the #api-navbar element. It looks like
all Fauxton pages do.
Adding in a DOM check to confirm the existence of the apibar
at this point...
https://github.com/apache/couchdb-fauxton/blob/master/app/addons/fauxton/base.js#L64
... has no effect because the core routeObject uses the
`two_pane.html` layout template as a base template when loading,
and that DOES contain the element. But due to the complexity of
the way our pages load, that template gets loaded first, THEN
the route object's template gets loaded. So basically we don't
know for sure whether the element exists until the moment it's
inserted/removed, hence the checks added in this PR.
I dedided to throw a warning in these cases. Seems like it should
be caught.
This fix should solve all of those Invariant Violation errors
of this kind, not just the problem with the API Bar.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/benkeen/couchdb-fauxton safety-check-add-remove-react-components
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-fauxton/pull/547.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #547
----
commit 2b4841e5bd5d5ba0989c7e7cdbf49bd2ba8261da
Author: Ben Keen <be...@gmail.com>
Date: 2015-10-02T18:51:18Z
Add DOM check before mounting/unmounting React components
The recent API Bar refactor unveiled a rather neat problem that
showed up in the Dashboard only. The Replication page failed to
load and threw a React "Invariant Violation" error which we've
seen show up from time to time but couldn't yet reproduce. What
was happening was that when attempting to mount the component,
the DOM element just didn't exist. The same problem
could occur when unmounting.
Now to go down the rabbithole! This may get boring, be warned...
The reason it occurred in the dash only is because some page
layouts don't contain the #api-navbar element. It looks like
all Fauxton pages do.
Adding in a DOM check to confirm the existence of the apibar
at this point...
https://github.com/apache/couchdb-fauxton/blob/master/app/addons/fauxton/base.js#L64
... has no effect because the core routeObject uses the
`two_pane.html` layout template as a base template when loading,
and that DOES contain the element. But due to the complexity of
the way our pages load, that template gets loaded first, THEN
the route object's template gets loaded. So basically we don't
know for sure whether the element exists until the moment it's
inserted/removed, hence the checks added in this PR.
I dedided to throw a warning in these cases. Seems like it should
be caught.
This fix should solve all of those Invariant Violation errors
of this kind, not just the problem with the API Bar.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Add DOM check before mounting/unmoun...
Posted by robertkowalski <gi...@git.apache.org>.
Github user robertkowalski commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/547#issuecomment-145489340
assuming there is no other test covering this codepath i would propose to fix the deleted test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Add DOM check before mounting/unmoun...
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/547#discussion_r41168996
--- Diff: app/core/tests/routeObjectSpec.js ---
@@ -197,17 +197,6 @@ define([
restore(React.unmountComponentAtNode);
});
- it('removes existing component via react', function () {
- var spy = sinon.stub(React, 'unmountComponentAtNode');
- var fakeSelector = 'remove-selector';
- testRouteObject.reactComponents[fakeSelector] = React.createElement('div');
--- End diff --
Bah, good point. Spent ages trying to construct a working test here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Add DOM check before mounting/unmoun...
Posted by robertkowalski <gi...@git.apache.org>.
Github user robertkowalski commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/547#discussion_r41129074
--- Diff: app/core/tests/routeObjectSpec.js ---
@@ -197,17 +197,6 @@ define([
restore(React.unmountComponentAtNode);
});
- it('removes existing component via react', function () {
- var spy = sinon.stub(React, 'unmountComponentAtNode');
- var fakeSelector = 'remove-selector';
- testRouteObject.reactComponents[fakeSelector] = React.createElement('div');
--- End diff --
you can just fix the existing test by creating the div with the fake selector
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Add DOM check before mounting/unmoun...
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/547#issuecomment-145601413
Test has been patched. I'll merge once green, thanks @robertkowalski!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Add DOM check before mounting/unmoun...
Posted by robertkowalski <gi...@git.apache.org>.
Github user robertkowalski commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/547#issuecomment-145488453
wow, thanks for the good writeup
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fauxton pull request: Add DOM check before mounting/unmoun...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-fauxton/pull/547
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---