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

Allow publish bundle to have React as external peer dependencies #2097

Closed
wants to merge 2 commits into from
Closed

Allow publish bundle to have React as external peer dependencies #2097

wants to merge 2 commits into from

Conversation

talves
Copy link
Collaborator

@talves talves commented Feb 14, 2019

Summary

Closes #2026

Changes:

Create new bundle in netlify-cms to use react and react-dom as externals

  • Move react and react-dom as peer deps in netlify-cms-core
  • Make react and react-dom dependencies in netlify-cms
  • Webpack config in netlify-cms to build new bundle

Test plan

Make sure netlify-cms works as espected.
Use new bundle in projects importing cms (like Gatsby starter)

A picture of a cute animal (not mandatory but encouraged)
🐶

@netlify
Copy link

netlify bot commented Feb 14, 2019

Preview proposed changes to netlifycms.org in the link below:

Built with commit 275c7a6

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

@netlify
Copy link

netlify bot commented Feb 14, 2019

Preview proposed changes to the CMS demo site in the link below:

Built with commit 275c7a6

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

@talves talves mentioned this pull request Feb 14, 2019
@erquhart
Copy link
Contributor

Discussed testing w/ @talves in Gitter, good to merge once confirmed working locally.

@talves
Copy link
Collaborator Author

talves commented Feb 18, 2019

@erquhart Making some changes to the build setup. I have some concerns/questions for you.

In the PR bundle netlify-cms-manual-init, the dependencies are not setup as peer dependencies in the package.json because the netlify-cms build needs them to be included to not break the build.
I am not sure how this is going to work with builds that don't recognize the peers because they are missing from the package.json. If I don't use yarn link, I don't have an issue, but not sure that is because I have resolutions for the two externals in my project. These tests have been a nightmare, because the error does not specify whether it is a duplicate react instance or an issue with hooks themselves.

My proposed changes (local: not pushed) solve the externals issue for using scripts.

  <script src="https://unpkg.com/react@16.8.2/umd/react.development.js"></script>
  <script src="https://unpkg.com/react-dom@16.8.2/umd/react-dom.development.js"></script>
  <script src="/cms/dist/netlify-cms-manual-init.js"></script>
  <script>
    var h = React.createElement;
    var createClass = CMS.createClass;
    CMS.init()
  </script>

I would like to take the globals into the CMS global (i.e. CMS.init, CMS.createClass) and leave the createElement on the React global. @erquhart Any objections?

Do you think not having the peer deps in the package.json for the library is going to be an issue when someone imports netlify-cms/dist/netlify-cms-manual-init like the Gatsby setup?

After thinking hard on this, I really believe this change should:

  • hoist the react and react-dom dependencies of the current netlify-cms.js bundle from core.
  • then we should create a new library that keeps the the react and react-dom dependencies as external peer dependencies.
  • once we go to 3.0, this new library would become the official netlify-cms library and ALL uses of netlify-cms would have to use it correctly. We deprecate it to netlify-cms on 3.0.

I can't think of a better way to handle this without creating a huge mess and support being a nightmare to track down issues.

@erquhart
Copy link
Contributor

I commented a bit on this in the hooks issue, but one more question from your last comment here.

  • once we go to 3.0, this new library would become the official netlify-cms library and ALL uses of netlify-cms would have to use it correctly. We deprecate it to netlify-cms on 3.0.

How does this work if someone wants to include netlify cms with a script tag and have it auto run as an SPA? And when using the script tag/SPA approach, would it be compatible with components using hooks?

@talves
Copy link
Collaborator Author

talves commented Feb 19, 2019

I have done extensive tests and spent quite a bit of time on it. I cannot find a solution that works without issues. This solution alone will not work with the script layout above. I tried a few things and none of them to my liking.

How does this work if someone wants to include netlify cms with a script tag and have it auto run as an SPA?

I envisioned it still existing in the solution. I don't think it would be an issue.

@talves talves closed this Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants