decap-cms icon indicating copy to clipboard operation
decap-cms copied to clipboard

feat: register custom formats take 2

Open mmkal opened this issue 4 years ago • 16 comments

Replaces #5839 Fixes #5848

Summary

Adds the ability to register custom formats (could be xml, po, binary, json5, etc.).

Also adds the requested tests and docs from the last PR (1dc43db87936fa480d3650972f2bf0a0882da726 + 6d29b239cb36073d4dde8d5bd0d3db87c86eab07)

Test plan

Unit tests for register function + formatters themselves. Then ran manual tests by doing npm pack, installed the .tgz file directly in the project we're using netlify-cms with, then registering some custom formats... and it worked.

Checklist

  • [x] I have read the contribution guidelines.
  • [x] Code is formatted via running yarn format.
  • [x] Tests are passing via running yarn test.
  • [x] The status checks are successful (continuous integration). Those can be seen below.

mmkal avatar Feb 12 '22 23:02 mmkal

@erezrokah any ideas why cypress is failing? I wasn't able to run them myself and don't have permission to download screenshots.

mmkal avatar Feb 16 '22 19:02 mmkal

Looks like our tests started failing with a specific renovate PR https://github.com/netlify/netlify-cms/actions/workflows/nodejs.yml?query=branch%3Amaster.

Looking into it

erezrokah avatar Feb 18 '22 13:02 erezrokah

@erezrokah using yarn test:e2e:dev the can change status on and publish multiple entries test was passing for me locally, so I don't think the failure is related to these changes, unless I'm misunderstanding something?

mmkal avatar Feb 19 '22 19:02 mmkal

@erezrokah using yarn test:e2e:dev the can change status on and publish multiple entries test was passing for me locally, so I don't think the failure is related to these changes, unless I'm misunderstanding something?

You're correct. I don't think the failure is related to your changes. However I'd prefer to fix the tests before merging this PR.

erezrokah avatar Feb 21 '22 10:02 erezrokah

@erezrokah thank you for helping get green! It says there's one workflow awaiting approval. Anything else I should do to get this over the line?

mmkal avatar Feb 22 '22 14:02 mmkal

It says there's one workflow awaiting approval

That's GitHub's crypto mining defense technic :) First committers need approval from a maintainer to run the CI.

I'm trying to think of a way to avoid manual initialization. We could have the custom formats defined as top level item config.yml and use that to validate, WDYT?

It will also make it clear what's supported

erezrokah avatar Feb 28 '22 15:02 erezrokah

It says there's one workflow awaiting approval

That's GitHub's crypto mining defense technic :) First committers need approval from a maintainer to run the CI.

I'm trying to think of a way to avoid manual initialization. We could have the custom formats defined as top level item config.yml and use that to validate, WDYT?

It will also make it clear what's supported

I'm not sure - wouldn't some custom javascript have to run to register the formatter implementation? Where would netlify-cms know where to find that javascript if there's no manual initialization. Also - I don't feel very comfortable adding non-manual initialization support since the team have been using it since day one.

Would you be open to it going in without, then if there are users requesting custom formats without wanting to manually initialize, a separate piece of work could be done to support that use case too? (Selfishly - that wouldn't help my team and we would love to be able to use this now!)

mmkal avatar Feb 28 '22 20:02 mmkal

Thanks for your input @mmkal, what concerns me is that the CMS default is not to have manual initialization so requiring it here breaks that. Also other register* methods don't require it.

Another option is to drop the static config level schema validation and only validate dynamically when trying to retrieve the format. We do the latter for custom widgets and display an "Unknown" widget when that happens.

WDYT?

erezrokah avatar Mar 01 '22 10:03 erezrokah

Thanks for your input @mmkal, what concerns me is that the CMS default is not to have manual initialization so requiring it here breaks that. Also other register* methods don't require it.

I'm not sure I agree it breaks that. The default setup still doesn't require manual initialisation - it's only required if you want to start pretty seriously customising the behaviour by adding extra file formats. Having said that, I can't see why it needs to be any different from registering a custom widget.

Another option is to drop the static config level schema validation and only validate dynamically when trying to retrieve the format. We do the latter for custom widgets and display an "Unknown" widget when that happens.

WDYT?

I think this is already happening thanks to this change:

image

If someone tries to use an un-registered widget, there will just be an error thrown. So maybe this is already meeting the requirements? Is there a simple way to test this with a non-manual-initialized setup?

mmkal avatar Mar 15 '22 14:03 mmkal

@erezrokah just tested this and it actually doesn't require manual initialization. It works similarly to the other CMS.register... methods.

It looks like this if you try to use a non-existent formatter:

image

So to address your two comments:

  1. Manual initialization: not required.
  2. Drop static validation: done. We now only validate that the prop is a string.

So, anything else needed for this to go in?? 🙏

mmkal avatar Apr 23 '22 15:04 mmkal

@erezrokah bump - anything I can do to get this in?

mmkal avatar May 12 '22 00:05 mmkal

Ping @erezrokah - CC @bytrangle in case Erez is no longer working on netlify-cms?

mmkal avatar Jun 07 '22 16:06 mmkal

pinging @krider2010

erezrokah avatar Jun 07 '22 17:06 erezrokah

@mmkal Thank you for the time you've put into this PR. I will find time to review this soon.

bytrangle avatar Jun 08 '22 09:06 bytrangle

Just a heads up I'm working on being added to the maintainer group for review reasons.

krider2010 avatar Jun 13 '22 13:06 krider2010

@bytrangle @krider2010 @erezrokah fixed the prettier problem - hopefully ready now but won't know til the build is approved! 🙏

mmkal avatar Jul 17 '22 17:07 mmkal

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Apr 26 '23 09:04 stale[bot]

I still want to merge this!

mmkal avatar Apr 27 '23 19:04 mmkal

Deploy Preview for decap-www ready!

Name Link
Latest commit aed01d0ef5aac25987499f2069e23a0d6c7d4b52
Latest deploy log https://app.netlify.com/sites/decap-www/deploys/6488a17230778a00081b11b3
Deploy Preview https://deploy-preview-6205--decap-www.netlify.app/docs/beta-features
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Apr 27 '23 19:04 netlify[bot]

@martinjagodic the only thing failing (after a year and a half of "Update from main and cross fingers") is Markdown widget breaks pressing enter:

image

I really don't think it's related - could we merge this as is? I've updated to incorporate all the netlify-cms-* to decap-cms-* package changes.

mmkal avatar Aug 23 '23 19:08 mmkal