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

chore(netlify-cms): update for v2, add standalone build system #6356

Merged
merged 6 commits into from
Jul 17, 2018

Conversation

erquhart
Copy link
Contributor

@erquhart erquhart commented Jul 10, 2018

Fixes #4516, #6048, #6248.

  • Update to v2 API's
  • Use a standalone webpack compiler to ensure against impacts on main site
  • Accept custom preview pane styles (even raw preprocessor styles) via plugin options
  • Optionally disable identity widget (ported from gatsby-plugin-netlify-cms: allow identity widget disabling #5942)
  • Make custom preview templates and styles work during local development
  • Added a few other small options (documented)

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 10, 2018

Deploy preview for gatsbygram ready!

Built with commit 23a4816

https://deploy-preview-6356--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jul 10, 2018

Deploy preview for using-drupal ready!

Built with commit 23a4816

https://deploy-preview-6356--using-drupal.netlify.com

@erquhart
Copy link
Contributor Author

Not sure why those failures are happening, not seeing them locally.

@m-allanson
Copy link
Contributor

Nice work, that's a much simpler gatsby-node.js! Adding Parcel seems like a fairly heavy dependency though - what are your thoughts on using the same onPostBuild approach to run a separate build, but configured with webpack's Node API instead of using Parcel?

Those test failures seem odd - I can replicate them by checking out this branch and doing the following:

yarn
yarn run bootstrap
yarn run test

I'll do some more digging to see if I can find what's causing the failures.

@erquhart
Copy link
Contributor Author

That's fair, I'll switch it to webpack.

@emanueleperuffo
Copy link
Contributor

I've created PR #6391

@erquhart
Copy link
Contributor Author

@emanueleperuffo I mentioned this on your PR, but THANK YOU so much for digging into this. Our efforts just happened to overlap as the Gatsby/Netlify CMS use case has been one of the "fires I've let burn" for a bit while tending to other things.

I took the opportunity to finish the v2 update, and also to fix longstanding issues with using custom preview pane styles during Gatsby local development.

There are a lot of known issues with this plugin, especially as documented in #4516, and I think this PR addresses all known issues while keeping the plugin minimally complex.

@m-allanson all finished here.

@erquhart erquhart force-pushed the topics/netlify-cms-v2 branch 5 times, most recently from 3c88c93 to c1fe8e1 Compare July 12, 2018 21:45
@erquhart
Copy link
Contributor Author

The test and deploy failures are present in other pull requests too, I don't believe any of them are related to this PR.


window.netlifyIdentity = netlifyIdentityWidget
netlifyIdentityWidget.init()
if (NETLIFY_CMS_PREVIEW_STYLES_SET) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The CI builds are failing because this is an undefined variable.

Copy link
Contributor Author

@erquhart erquhart Jul 13, 2018

Choose a reason for hiding this comment

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

Ah, I see that now - it's not actually undefined, but failing linting. Weird that it passes locally.

@erquhart
Copy link
Contributor Author

@m-allanson @KyleAMathews I don't believe this PR is responsible for any test failures - can this be merged?

@KyleAMathews
Copy link
Contributor

Hmm can't recreate the test errors locally... @m-allanson or @pieh — any idea what those test errors mean?

@m-allanson
Copy link
Contributor

It turns out that React isn't included as a dependency anywhere in the repo. The netlify-cms package depends on React, but that is now a peerDependency. I guess this wasn't showing up locally as the old React packages would still be hanging out in node_modules

@erquhart I got this working by adding react and react-dom as devDepencies of packages/gatsby. I have a commit locally but don't have access to push it to your repo. Feel free to create your own commit for this instead.

@m-allanson
Copy link
Contributor

this PR addresses all known issues while keeping the plugin minimally complex.

Just wanted to add a big 🎉 for this :)

What is the relation between this PR and #6391? Should just this one be merged?

@shaunbent
Copy link
Contributor

Looking forward to seeing this PR merged, this is currently blocking us from upgrading our site to Gatsby V2.

Thanks for the awesome work @erquhart 🎉😎

@KyleAMathews
Copy link
Contributor

Ahhhh! That makes sense @m-allanson! Weird though you can't push to his repo as I can 🤔

Anyways, I'll make the change and push it right now.

Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Yeah, tests are passing. Thanks @erquhart for the nice PR!

@KyleAMathews KyleAMathews merged commit ddc9b80 into gatsbyjs:master Jul 17, 2018
@KyleAMathews
Copy link
Contributor

Published PR to gatsby-plugin-netlify-cms@2.0.0-beta.6

@erquhart erquhart deleted the topics/netlify-cms-v2 branch July 17, 2018 18:15
@erquhart
Copy link
Contributor Author

@m-allanson @KyleAMathews good catch on the dependency issue, thank you both so much for digging in.

@steverandy
Copy link

Previously we had this line

netlifyIdentityWidget.on(`login`, () => {
  document.location.href = `/admin/`
})

Is there any reason why now it's changed to reloading the page?

@ascorbic
Copy link
Contributor

It seems that this is no longer transpiling TypeScript. My preview templates are TypeScript, and previously this was fine because it used the webpack config from the main site, which includes the TypeScript plugin. Now they're failing to compile. Is there a way of exposing the Netlify Webpack config?

@erquhart
Copy link
Contributor Author

@steverandy I was actually in the middle of troubleshooting why reloads weren't happening on login, that needs to be changed.

@ascorbic the Netlify CMS webpack config uses any plugins that loaded before the Netlify CMS plugin itself. Try moving your Typescript plugin before the Netlify CMS plugin and see if that works. It would be worth considering how to capture all plugins used, regardless of placement, but the current setup should work for most.

@emanueleperuffo
Copy link
Contributor

Is it possible to use version > 2.0.1 like 2.0.1-beta.6 so npm doesn't update the package to 2.0.1 which is for gatsby v1?

@pieh
Copy link
Contributor

pieh commented Jul 20, 2018

@emanueleperuffo Hmmm, we should bump major version for v2 - we did that some time ago for packages in this repository, but it seems like we have missed this package

@ascorbic
Copy link
Contributor

Is anyone able to get this to generate the styles.css for custom preview styles? I opened a separate issue, #6641, as I thought it may have been a documentation issue, but I'm starting to think it may be a problem with the webpack config.

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.

9 participants