-
Notifications
You must be signed in to change notification settings - Fork 11
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
breaking: conversion to v2 addon format #390
Conversation
f88c180
to
d361b82
Compare
7b281d4
to
1ff4104
Compare
@MelSumner I think this is ready to go, and hopefully CI passes, I have tested everything locally |
a8b7be9
to
b2d48a0
Compare
I'll do my best to review this ASAP, can you make sure to update the PR description with the changes made? |
Question: Do we need to do something similar in order for the |
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.
Three things:
- Please review the test that is failing
- Check the updated description, I gave it a go but there may be more we wish to include.
- Please add instructions to the README for how users need to import the CSS for the addon; I think that would be appropriate for this PR, since that changed in this PR.
Anything else that was a nice-to-have has been moved to separate issues.
Thank you so much! Can't wait to get this out as a new release!
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.
Approving this PR with the expectation that the sole outstanding floating-dependencies
issue will be resolved before merge + release. It looks great -- both my and and @MelSumner 's change requests have either been made or appropriately scoped elsewhere to my satisfaction.
👍
41d4f09
to
045e390
Compare
addon/.stylelintignore
Outdated
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 looking into this -- I have approved the CI run.
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.
No luck -- I'll take a closer look at this against the latest v2 addon blueprint for stylelint deps / versions / config.
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.
I have a idea that it might be relate to pnpm v10 and --no-lockfile
and verify-deps-before-run
options, it might have been removed
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.
yep, here we go pnpm/pnpm#8865
it seems solution is in main but not release, I would say we can either skip this part of CI so we can merge this PR and them unskip when pnpm releases a new version which should not take too long, or just wait till release and then merge this PR. Optionally disable verify-deps-before-run
flag for now
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.
Happy to see the fix has been identified and merged upstream. I'd hope for a quick turnaround for a release with our blocker fixed in there. Pnpm shipping a larger, slower release with a wave of fixes for the new major would be understandable.
Question: what do you think about filing this issue as a blocker for cutting our next release, but not letting it block this PR?
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.
Yeah it's not blocking at all, just nice to have. I have commented out this flag for now, since CI is green I think we are gtg, I will add some readme update in a bit + create follow up PR for uncommenting pnpm flag
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.
Question: is there any reason we can't keep pnpm@9 and ship both the PR and release immediately?
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.
you can ship immediately on pnpm@v10, without follow up PR as it's not really important for end consumer
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.
Ok, I think this is good for now, we'll follow up with a few more PRs before cutting a release. Thank you for your contributions!!
If merged, this PR converts this addon to the V2 addon format. Resolves #359, #372, and #380.
Details
navigation-narrator
component togts
addon/src
folderBreaking Changes