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

Add Decorator Support #428

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alexlafroscia
Copy link
Contributor

@alexlafroscia alexlafroscia commented Apr 5, 2020

This adds two decorators, memberAction and collectionAction, that are exported from ember-api-actions/decorators. They are built to be compatible with the V1 decorator spec.

The exported functions are called with the same options as the existing helpers, and internally call the exact same logic.

Open Questions:

  • Is it worthwhile to modify the existing functions instead to detect usage as a decorator, as to avoid exporting a second set of helper methods?
  • Should the documentation be updated to default to the decorator-based usage?

Left To-Do, Pending Question Resolution:

  • Add documentation about decorator usage

Closes #413

@alexlafroscia
Copy link
Contributor Author

@dbollinger I'd love to get your thoughts on the "open questions" above -- especially whether we should detect usage inside one version of memberAction and collectionAction or keep them as two separate imports that are designed for specific purposes.

@dbollinger
Copy link

dbollinger commented Apr 6, 2020

This is great!

For question 1 -- I generally favor the advice to "prefer duplication over the wrong abstraction" and in this case a separate export seems fine. Alpha/beta releases can help make that a smoother process as well.

For question 2 -- I think some additive documentation would be a good first step, but I'm not sure about the best way to manage that alongside the release process.

@alexlafroscia
Copy link
Contributor Author

prefer duplication over the wrong abstraction

Good words to live by! I’m cool with making it a separate import right now and maybe combining them sometimes in the future. This package is technically still pre-1.0.0, so the API can break before an official 1.0.0 release if it needs to!

Given the fact that it won’t be the exports from the “main” module, I’ll add some README documentation to for the decorator usage rather than updating the existing documentation to use the new flavor.

@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@ff24733). Click here to learn what that means.
The diff coverage is n/a.

@alexlafroscia
Copy link
Contributor Author

Rebased to use setupPretender again

@alexlafroscia
Copy link
Contributor Author

Hmm... So it seems like the decorator tests don't working for Ember... 2.16 and 2.18.

Some thoughts:

  • Should we bother with those Ember versions at all? We could raise the minimum version run against to the most recent LTS going forward
  • We could just skip the decorator tests for these old Ember versions

@emilkarl
Copy link

Hmm... So it seems like the decorator tests don't working for Ember... 2.16 and 2.18.

Some thoughts:

  • Should we bother with those Ember versions at all? We could raise the minimum version run against to the most recent LTS going forward
  • We could just skip the decorator tests for these old Ember versions

Will decorators work in those versions anyways? 🤷‍♂️

@alexlafroscia
Copy link
Contributor Author

Will decorators work in those versions anyways? 🤷‍♂️

They could -- the original implementation of the decorator I had did -- but the way it is not implemented, where TypeScript's compiler is satisfied, does not.

I would be in favor of just dropping support for these two very old Ember versions and increase the minimum version to 3.12. I will address that in a separate PR and we can discuss the idea more there.

@esbanarango
Copy link

@mike-north @alexlafroscia do we know what's the status here 🙏

@alexlafroscia
Copy link
Contributor Author

No updates from me 🤷‍♂️ I honestly forgot I made these PRs, but with the update-to-Ember-3.12-at-a-minimum PR still standing as it is, I don't think we're in a place to move forward really.

@ducharmemp
Copy link

Looks like that Pr landed so this PR can move forward? I think the prototype workaround is alright but decorators would also be nice!

@MarcoKramer
Copy link

Is there any news on this? Would indeed be nice to have this in.

@snewcomer
Copy link

@alexlafroscia Possible we could rebase with the latest on the main branch and see if @Turbo87 would be open to merging and releasing it?

@esbanarango
Copy link

That would be 🔝 @alexlafroscia @snewcomer !! Thank you for putting this together 🙏

@jkeen
Copy link

jkeen commented Nov 21, 2022

@snewcomer Did the rebase and made a new PR which is here: #609

@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 21, 2022

FWIW we (@mainmatter) forked the addon and created a new API that does not involve decorators at all but should also be a bit more TS friendly once we add types: https://github.com/mainmatter/ember-api-actions

@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 21, 2022

also, even if we merged this PR we still couldn't release it, because @mike-north still hasn't given anyone npm ownership of the package... :-/

@jkeen
Copy link

jkeen commented Nov 21, 2022

@Turbo87 Thanks for forking and taking over! I hate it when this happens to great projects (and great PRs) and there's no clear passing of the torch.

Your solution feels great, and I'm gonna migrate over to that and forget about this branch entirely! 🎉

@jkeen
Copy link

jkeen commented Nov 21, 2022

@mike-north Pssssst think you could pass the keys over to someone else and archive this repo, maybe updating the readme with a link to the maintained project? Seems like you're not into this project anymore and it'd be great to give the community a direction to go towards.

@jkeen
Copy link

jkeen commented Nov 22, 2022

In trying to use your version @Turbo87, it looks like…

a) it's not actually a fork of this project but a completely new project with the same name? If you're looking to carry this project forward into the future (and I hope you are!), it feels weird to not have the years of git history that came before. If this were my project that I started and then abandoned, I'd feel slighted if my name were erased from the history.

b) Your addon seems to be missing some key features that this one has, like the before and after hooks on the actions. Was that intentional?

@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 22, 2022

it doesn't have the history because we've rewritten it from scratch with a completely different API 😅

yes, before and after missing is intentional, because you can just do those before and after the action call within the async function.

also, I apologize for the lack of documentation so far. it's still work in progress, but I thought I'd share it here already anyway :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document "Octane" usage
9 participants