search

miguelcobain / ember-yeti-table

Yeti Table
https://miguelcobain.github.io/ember-yeti-table
MIT License
61 stars 15 forks source link

V2 Addon format #462

Closed AmauryD closed 7 months ago

AmauryD commented 9 months ago

Related issue: #451

AmauryD commented 9 months ago

Hello @miguelcobain,

Here's the first draft. I had a few questions that I made in the review.

AmauryD commented 9 months ago

@miguelcobain ready for review ! 🎉

cah-brian-gantzler commented 9 months ago

Where is the helper EQ used at? Docs? Yeti? Both? If Docs or Yeti, I would think that there would not be a addon/-private/helpers dir off the root, but would exist in the correct place. If both, I think I would just define the helper in yeti and in docs. Its so small. But pretty sure having it outside of the respective app is a bad thing.

The app dir is no longer used to get the component merged with the consuming application, there is a section in the package.json that says what is merged. For examples see https://github.com/cibernox/ember-power-select/blob/master/ember-power-select/package.json#L156 and https://github.com/cibernox/ember-power-select/blob/master/ember-power-select/package.json#L156

Same would go for the test directory, testing the eq helper would be done with testing the results of the eq in yeti proper (if that is where it ended up). Although In the test app there would could a test for the helper since it is exported publicly to be merged with the app. Always hated that helper, its defined as private, but publicly exported. Wanted to just do away with the helper and write it another way..... or just depend on ember-truth-helpers.

cah-brian-gantzler commented 8 months ago

There are so many files changed I misread the pull request. Looking at your repo directly I see my mistake. I believe all my statements are incorrect.

Will use your branch soon to see if I see any issues and report back. I think we may be god to go.

Ember-resources 7.0 contains a breaking change in that the usage of keepLatest has been moved https://github.com/NullVoxPopuli/ember-resources/blob/main/MIGRATIONS.md#70. Should maybe we work that change into this as well?

EDIT: There seems to be a problem with V2 addons and some V1 addons referring to addons that need to be rewritten by embroider. Because of this I am unable to update to ember-concurrency 4.0 because this addon is dependent on ember-resources 6.x. Thinking that updating to V2 of ember-resources will at least combine all the breaking changes into one release.

AmauryD commented 8 months ago

There are so many files changed I misread the pull request. Looking at your repo directly I see my mistake. I believe all my statements are incorrect.

I was indeed a bit confused by your comment. Because most of the files you were talking about were re/moved

Ember-resources 7.0 contains a breaking change in that the usage of keepLatest has been moved https://github.com/NullVoxPopuli/ember-resources/blob/main/MIGRATIONS.md#70. Should maybe we work that change into this as well?

I will take a look. I will also make some deps upgrade, it's been 2 months since this PR. Maybe some of the patches i made on ember-cli-docs were fixed.

AmauryD commented 8 months ago

Upgraded to ember-resources 7 and ember-concurrency 4.

There were many things to upgrade, including deprecations for @ember/runloop and for gjs templates. These may require reworking some parts of the addon, which I will address in another PR. https://github.com/miguelcobain/ember-yeti-table/blob/67c95275f7b5e4fcecbde51c41b0f1d47d1ae398/ember-yeti-table/.eslintrc.js#L56

https://github.com/miguelcobain/ember-yeti-table/blob/67c95275f7b5e4fcecbde51c41b0f1d47d1ae398/test-app/.eslintrc.js#L79-L80

Also, is dependency-lint package used in test-app still useful with pnpm ?

cah-brian-gantzler commented 8 months ago

Sorry to confuse you. I had made some comments and was saying they were wrong. I updated the original comment and struck through the comments.

Thanks for updating the ember-resources. For ember concurrency there is some weird play happeneing between ember-resources. reactiveweb, and ember-concurrency. I cant tell if all need to be upgraded at once in a consuming app or not. I am making an assumption that upgrading concerency is also the right thing. Resources and concurency are also changed to V2 addons in those releases.

I was going to try this version of yeti in my app and see how it works out and the problem with V2 addons is you cant just point to a different branch and try it out. Because of that, Kate created a workflow that posts the dist from the master build to branch so at least people can try the unreleased master. You can find that being implemented here https://github.com/bgantzler/ember-mirage/blob/main/.github/workflows/ember-mirage-dist.yml. @NullvoxPopuli implemented that there so not sure exactly, but looks pretty straight forward. If we were able to implement that here, I would be able to point to ember-yeti-table-dist on your repo and test the V2 version in my app. What do you think?

AmauryD commented 8 months ago

Good idea ! I published the dist with the action. It is available in https://github.com/AmauryD/ember-yeti-table/tree/master-dist branch ! 😄

cah-brian-gantzler commented 8 months ago

I will try pointing to the branch and see what I get. Currently I have one more addon ember-stargate that may cause a problem with ember-resources 7, will let you know if I can work through that and the result of using yeti-table V2

cah-brian-gantzler commented 8 months ago

Looking awesome. I have a couple PRs against my app updating some other addons. Once I get those applied tomorrow, then try this again to see if there are any conflicts.

cah-brian-gantzler commented 8 months ago

@miguelcobain Can we get this workflow run?

I have experimented with this version in my own application and everything is working great. Could you have a look at it and see if you have any questions. Im ok with approving it after the workflow runs and there are no issues.

miguelcobain commented 8 months ago

@AmauryD @cah-brian-gantzler as far as I can see, ember-concurrency is only used in tests and in the test app. Why are we bringing it to the addon's package.json dependencies?

Same question for @ember/test-waiters.

Btw, tests are green. 👍

AmauryD commented 8 months ago

@miguelcobain that's true for ember-concurrency. reactiveweb has a peerDependency on @ember/test-waiters. Thank's for the CI !

cah-brian-gantzler commented 8 months ago

Good catch on ember-concurrency. Wasnt looking that close. I have ember-concurrency in my app already so might not have noticed. Interestingly reactive web does have some conde that uses ember-concurrecny but does depend on it. https://github.com/universal-ember/reactiveweb/blob/main/reactiveweb/src/ember-concurrency.ts#L17

cah-brian-gantzler commented 8 months ago

@miguelcobain could you approve the work flow again and take another quick look please

cah-brian-gantzler commented 7 months ago

@miguelcobain bump. Would like to get this completed so that we could move onto fixing the reported bugs.

My bad. The work flow has run.

See any issues or can this be merged?

miguelcobain commented 7 months ago

@cah-brian-gantzler @AmauryD I just merged this. Thank you so much!

This warrants a major bump, right?

@cah-brian-gantzler should I wait on other bug fixes before releasing?

cah-brian-gantzler commented 7 months ago

Yes this is a major bump as its now a V2 addon and that requires people to meet certain criteria (auto-import for example)

I would release this, the bug fixes can be a minor release when done.

This includes the update to ember-resources (which is blocking me from updating ember-concurrency LOL) so getting a version out would be good.

Thanks @AmauryD

Someone else wanted to update this to typescript as well

cah-brian-gantzler commented 7 months ago

I update my app dependencies once a month, so will be a couple weeks til I have this in my app. I did test with @AmauryD version in a branch and found no issues

AmauryD commented 7 months ago

@cah-brian-gantzler @AmauryD I just merged this. Thank you so much!

This warrants a major bump, right?

@cah-brian-gantzler should I wait on other bug fixes before releasing?

You're welcome! Thanks to you too, @cah-brian-gantzler and @miguelcobain, for your review!

I'll start the conversion to TypeScript soon, as said in #461.

cah-brian-gantzler commented 7 months ago

@miguelcobain in case you were not following along. V2 addons are included into a build via the dist format. For a V2 addon you can not point to a branch, or master, and consume the addon. Part of the github actions added is that anytime something is merged into master, a branch master_dist is updated with the dist format from the master branch. This way someone can consume and test unreleased code (from the master anyway. Not sure what the protocol is for an adhoc branch for a wip). Please do not delete the master_dist branch.

I have changed my app to consume the master_dist and all is working well. Will wait til an NPM release is done then officially update the app to use the NPM version.