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

feat: register custom formats take 2 #6205

Merged
merged 35 commits into from
Aug 24, 2023

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Feb 12, 2022

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 (1dc43db + 6d29b23)

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

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

@mmkal mmkal requested a review from a team February 12, 2022 23:08
@mmkal mmkal mentioned this pull request Feb 13, 2022
4 tasks
@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Feb 14, 2022
@mmkal
Copy link
Contributor Author

mmkal commented Feb 16, 2022

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

@erezrokah
Copy link
Contributor

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

@mmkal
Copy link
Contributor Author

mmkal commented Feb 19, 2022

@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?

@erezrokah
Copy link
Contributor

@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.

@mmkal
Copy link
Contributor Author

mmkal commented Feb 22, 2022

@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?

@erezrokah
Copy link
Contributor

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

@mmkal
Copy link
Contributor Author

mmkal commented Feb 28, 2022

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!)

@erezrokah
Copy link
Contributor

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?

@mmkal
Copy link
Contributor Author

mmkal commented Mar 15, 2022

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
Copy link
Contributor Author

mmkal commented Jul 17, 2022

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

@aymenha aymenha mentioned this pull request Feb 2, 2023
@stale
Copy link

stale bot commented Apr 26, 2023

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 stale bot added the status: stale label Apr 26, 2023
@mmkal
Copy link
Contributor Author

mmkal commented Apr 27, 2023

I still want to merge this!

@netlify
Copy link

netlify bot commented Apr 27, 2023

Deploy Preview for decap-www ready!

Name Link
🔨 Latest commit aed01d0
🔍 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.

@mmkal
Copy link
Contributor Author

mmkal commented Aug 23, 2023

@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.

@martinjagodic martinjagodic enabled auto-merge (squash) August 24, 2023 07:22
@martinjagodic martinjagodic merged commit b3d41ea into decaporg:master Aug 24, 2023
@mmkal mmkal deleted the mk/custom-formatters branch August 25, 2023 12:42
martinjagodic pushed a commit that referenced this pull request Oct 16, 2023
* feat: register custom formats

* fix: register custom formats validation

* fix: change 'format' field validation to string instead of enum

Format will have the same behaviour as the widget property

* test: custom formats and register function

* docs: explain usage and note manual initialization requirement

* fix: remove unused imports

* use default extension

* remove manual init note

* PR comments

* fix: prettier

* revert unnecessary changes?

* chore: more revert?

* chore: newline

* chore: update import

---------

Co-authored-by: Jean <jlabedo@gmail.com>
Co-authored-by: Misha Kaletsky <mmkal@users.noreply.github.com>
Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
martinjagodic pushed a commit that referenced this pull request Oct 17, 2023
* feat: register custom formats

* fix: register custom formats validation

* fix: change 'format' field validation to string instead of enum

Format will have the same behaviour as the widget property

* test: custom formats and register function

* docs: explain usage and note manual initialization requirement

* fix: remove unused imports

* use default extension

* remove manual init note

* PR comments

* fix: prettier

* revert unnecessary changes?

* chore: more revert?

* chore: newline

* chore: update import

---------

Co-authored-by: Jean <jlabedo@gmail.com>
Co-authored-by: Misha Kaletsky <mmkal@users.noreply.github.com>
Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
martinjagodic pushed a commit to geotrev/netlify-cms that referenced this pull request Oct 17, 2023
* feat: register custom formats

* fix: register custom formats validation

* fix: change 'format' field validation to string instead of enum

Format will have the same behaviour as the widget property

* test: custom formats and register function

* docs: explain usage and note manual initialization requirement

* fix: remove unused imports

* use default extension

* remove manual init note

* PR comments

* fix: prettier

* revert unnecessary changes?

* chore: more revert?

* chore: newline

* chore: update import

---------

Co-authored-by: Jean <jlabedo@gmail.com>
Co-authored-by: Misha Kaletsky <mmkal@users.noreply.github.com>
Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom output formatter
6 participants