Skip to content

Add Decorator Support - #428

Open
alexlafroscia wants to merge 4 commits into
mike-north:masterfrom
alexlafroscia:decorator-support
Open

Add Decorator Support#428
alexlafroscia wants to merge 4 commits into
mike-north:masterfrom
alexlafroscia:decorator-support

Conversation

@alexlafroscia

@alexlafroscia alexlafroscia commented Apr 5, 2020

Copy link
Copy Markdown
Contributor

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

Comment thread tests/helpers/setup-pretender.ts Outdated
@alexlafroscia

Copy link
Copy Markdown
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

dbollinger commented Apr 6, 2020

Copy link
Copy Markdown

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
Copy Markdown
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

codecov Bot commented Apr 7, 2020

Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

Rebased to use setupPretender again

Comment thread tests/unit/utils/decorators/collection-action-test.ts Outdated
@alexlafroscia

Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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

@alexlafroscia

Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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

@snewcomer

Copy link
Copy Markdown

@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
Copy Markdown

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

@jkeen

jkeen commented Nov 21, 2022

Copy link
Copy Markdown

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

@Turbo87

Turbo87 commented Nov 21, 2022

Copy link
Copy Markdown
Collaborator

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

Turbo87 commented Nov 21, 2022

Copy link
Copy Markdown
Collaborator

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

jkeen commented Nov 21, 2022

Copy link
Copy Markdown

@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

jkeen commented Nov 21, 2022

Copy link
Copy Markdown

@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

jkeen commented Nov 22, 2022

Copy link
Copy Markdown

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

Turbo87 commented Nov 22, 2022

Copy link
Copy Markdown
Collaborator

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