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

Web-components: Custom Elements Manifest v1 support #15138

Merged
merged 14 commits into from Jun 29, 2021
Merged

Web-components: Custom Elements Manifest v1 support #15138

merged 14 commits into from Jun 29, 2021

Conversation

thepassle
Copy link
Contributor

Issue:

What I did

This adds support for the V1 version of the Custom Elements Manifest schema. This change is non breaking

TODO:

  • I'd like to test the changes locally, but I havent figured out how to make that happen (as mentioned on discord)

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jun 4, 2021

Nx Cloud Report

CI ran the following commands for commit 56d28eb. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@gaetanmaisse
Copy link
Member

@thepassle feels free to ping me on discord if you need help I would be glad to try to help 😉

@gaetanmaisse
Copy link
Member

@thepassle I think you were just an export away 😉

I push a couple of commits to add JSDoc on the component used in the WC kitchen sink, add an NPM Script to run the analyzer easily and it looks like it is working:
image

On the screenshot you will see that the descriptions are empty for the properties even if I added some in the JSDoc, I think it's because I activated the --litelement option and so we are ending with multiple times the same property in the manifest:

I think we should merge these 2 objects describing the same property. Do you think it's something that could/should be changed on @custom-elements-manifest/analyzer side?

Notes: I set a specific version of TypeScript using Yarn's resolution because we need to update Yarn before being able to us the latest version, this isn't related to this PR.

@thepassle
Copy link
Contributor Author

thepassle commented Jun 19, 2021

On the screenshot you will see that the descriptions are empty for the properties even if I added some in the JSDoc, I think it's because I activated the --litelement option and so we are ending with multiple times the same property in the manifest:

Thats odd — that should not be the case. Let me doublecheck this in the analyzer
Edit: Yeah, this is a bug on my side. Working on a fix 🙂

@thepassle I think you were just an export away 😉

Doh 🤦‍♂️🤦‍♂️🤦‍♂️

@thepassle
Copy link
Contributor Author

@gaetanmaisse The duplicate fields appearing the manifest bug should be fixed in @custom-elements-manifest/analyzer@0.3.1. I forgot to add a check to see if there was already an existing field with the same name in the manifest. Thanks for pointing it out 😄

I think we also need to add some docs to document how to use the new version of the manifest, do we need anything else to move forward with this PR?

@thepassle
Copy link
Contributor Author

@gaetanmaisse @shilman I've updated the documentation 🙂

@gaetanmaisse gaetanmaisse changed the title WIP: feat: support Custom Elements Manifest v1 Web-components: Custom Elements Manifest v1 support Jun 29, 2021
@@ -47,26 +48,30 @@
"@storybook/source-loader": "portal:../../lib/source-loader",
"@storybook/theming": "portal:../../lib/theming",
"@storybook/ui": "portal:../../lib/ui",
"@storybook/web-components": "portal:../../app/web-components"
"@storybook/web-components": "portal:../../app/web-components",
"typescript": "4.2.4"
Copy link
Member

Choose a reason for hiding this comment

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

Notes: I set a fixed version to avoid compat' issue with the Yarn 2 version we are using for now

Copy link
Member

@gaetanmaisse gaetanmaisse left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Notes: e2e errors are not related to this PR, see #15411

@gaetanmaisse gaetanmaisse merged commit 88e5774 into storybookjs:next Jun 29, 2021
@gaetanmaisse
Copy link
Member

Merged! Thanks for the PR @thepassle and let see what could be the next improvements on that topic :)

@thepassle
Copy link
Contributor Author

Awesome! Thanks so much for the help on this 🙂 Do you have any indication on when this will get released?

@shilman
Copy link
Member

shilman commented Jun 29, 2021

6.4-alpha = release later today
6.4-final = september

see #15355

@edoardocavazza
Copy link

edoardocavazza commented Jun 30, 2021

Hello!

It seems that it does not handle the privacy field, so it also processes private properties. Is this the correct behaviour or should I open an issue for that?

@shilman
Copy link
Member

shilman commented Jun 30, 2021

@edoardocavazza new issue please! 🙏

@jonniebigodes
Copy link
Contributor

@thepassle one final thing I would like to ask of you. If you don't mind popping into our Discord Server and message me (same username) to follow up on a couple of items.

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

5 participants