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

auto/manual init w/ single bundle, BYO root element 🎉 #1173

Merged
merged 3 commits into from
Mar 28, 2018
Merged

Conversation

erquhart
Copy link
Contributor

No description provided.

@verythorough
Copy link
Contributor

verythorough commented Mar 14, 2018

Deploy preview for netlify-cms-www ready!

Built with commit 4caba01

https://deploy-preview-1173--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Mar 14, 2018

Deploy preview for cms-demo ready!

Built with commit 4caba01

https://deploy-preview-1173--cms-demo.netlify.com

@erquhart erquhart force-pushed the init-once branch 2 times, most recently from 64c35a7 to 60a1089 Compare March 19, 2018 14:34
@erquhart erquhart changed the title ensure that application is only initialized once auto and manual init from a single bundle, plus BYO root element 🎉 Mar 19, 2018
@erquhart erquhart changed the title auto and manual init from a single bundle, plus BYO root element 🎉 auto and manual init from a single bundle, BYO root element 🎉 Mar 19, 2018
@erquhart erquhart changed the title auto and manual init from a single bundle, BYO root element 🎉 auto/manual init w/ single bundle, BYO root element 🎉 Mar 19, 2018
@erquhart erquhart force-pushed the init-once branch 2 times, most recently from eca9877 to c40636f Compare March 19, 2018 15:03
Copy link
Collaborator

@talves talves left a comment

Choose a reason for hiding this comment

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

Need to fix this line.
Testing now 👍

I like these changes better than mine 😄

src/bootstrap.js Outdated
if (document.getElementById(ROOT_ID)) {
console.error('Bootstrap attempted, but Netlify CMS is already initialized!');
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't believe this is a good practice, because we should allow for the existance of <div id="nc-root" />, so we can mount the cms to a rendered element in another app.

Suggestion: We check for the bootstrap trying to render onto an already rendered CMS instead. (ie an error calling init() twice in a row without the unmount of the CMS.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you were looking at an old commit, this was removed.

src/bootstrap.js Outdated
*/
const newRoot = document.createElement('div');
newRoot.id = ROOT_ID;
document.body.appendChild(el);
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs to be
document.body.appendChild(newRoot);

@erquhart
Copy link
Contributor Author

@talves after you came up with the idea of using a DOM element to trigger manual init, I took a second look at the whole thing and added a commit to really simplify everything by separating manual init and using a custom mount element. We can just use a global flag for manual init so that a custom element is optional.

Using a single bundle removes the risk of dual init, so we don't need those warnings anymore. Removing that apparatus also made resetting as simple as rerunning the init function, so it's pretty much gloves off if you use manual init, no safety rails :)

Let me know what you think.

@talves
Copy link
Collaborator

talves commented Mar 19, 2018

@erquhart I really like the simplification, but it introduced a side affect in the create-react-app test.

I will take another look to see why and try to get back to you soon

@erquhart
Copy link
Contributor Author

Testing now, looks like there are a few errors.

@erquhart
Copy link
Contributor Author

Fixed.

@talves
Copy link
Collaborator

talves commented Mar 19, 2018

Ok, all tested and working as expected. 👍
NOTE: Make sure to use the correct global flag 🤣

@erquhart erquhart merged commit a19bc04 into master Mar 28, 2018
@tech4him1 tech4him1 deleted the init-once branch March 29, 2018 20:54
brianlmacdonald pushed a commit to brianlmacdonald/netlify-cms that referenced this pull request May 23, 2018
* ensure that application is only initialized once

* allow for single bundle init

* enable manual initialization via global flag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants