-
Notifications
You must be signed in to change notification settings - Fork 248
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
plugin(npm): create a standalone demo application for npm plugin #1516
Conversation
Changed Packages
|
0bc51fe
to
3b106ba
Compare
Thanks for the contribution! |
9685ae2
to
2aaa938
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tiny code style ideas :)
a5575ef
to
10bdf30
Compare
10bdf30
to
cb5fa84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that change. Lgtm!
cc: @nickboldt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the adjustments @karthikjeeyar, just one change needed around the changeset config 👍
55631e4
to
051f7a5
Compare
This PR now contains only the standalone demo application, I have removed all the release related changes. |
051f7a5
to
56a5834
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @karthikjeeyar, lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @karthikjeeyar, thanks for the changes to match things going on outside this PR impacting you. 🙏
Looking over this again and I left a comment about the exports in your plugin.ts
, you also need to add a changeset as the code is changing and you'll want a new package version published to NPM.
Signed-off-by: Karthik Jeeyar <[email protected]>
56a5834
to
4227c18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid! Thanks again @karthikjeeyar 🚀
Actually, this breaks the existing code because it changes the exported names. :-/ The direction is fine, of course; the usage of And I'm thankful for the code improvement and review! But at the same moment, it's a bit sad that I didn't had "the chance" to review this. |
Hi @christoph-jerolimov, sorry, you are absolutely correct. In this case I moved too fast. How best should we move forward? I'm willing to put a PR together if needed. |
It's fine! I don't expect any users at the moment 😏 🤷♂️ I just wanted share "that feeling" and thanks that you accepting the feedback. 🤗 Moving fast, breaking things and fixing them is fine. I will look into that report.api.md issue and raise a PR later today, but we don't need to rush on that for me. |
Thanks, it for sure was not intentional, I had some time to review and this looked ready to go. There are 117 downloads for this package, people are using it and for them this would be a breaking change. Do you want me to at least update the changelog to match that reality? |
I have the code change waiting in a branch here: main...awanlin:community-plugins:topic/npm-changelog-correction, if you want me to make a PR just let me know. |
Fine for me. I thought about exporting Entity*Card as *Card as well, and marking it as deprecated. (Work in progress) |
It's your plugin and I'm tryin to correct my mistake, if the deprecated path if the one you want to take I'm fine with that 👍 |
How does this look to you? main...awanlin:community-plugins:topic/npm-correction |
You're fast :) I worked on a similar idea, wdyt about #1751? |
Actually, I'm confused why the renaming was required. Last week, when I checked this with Karthik we both had issues with the api exports which I couldn't reproduce.. And I'm not sure if its a best practice to prefix entity Cards with Entity* |
#1751 looks good to me, I've approved but leaving it to you to tell me when to merge. As for the |
Apologies for introducing breaking changes, my intent was to follow Entity* naming convention which helps to clarify that these component needs to be wrapped inside |
This PR adds a standalone npm demo application.
How to test:
yarn start
✔️ Checklist
Signed-off-by
line in the message. (more info)