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

When using the proper decorator syntax with the babel fix it errors wanting the task to be a getting #301

Closed
robclancy opened this issue Jun 3, 2019 · 8 comments

Comments

@robclancy
Copy link

robclancy commented Jun 3, 2019

When using this syntax

  @task({ drop: true })
  *loadSession() {
    ...
  }

With the fix from here (babel/babel#9912) I get an error about it wanting to be a getter (I assume you extend the @computed property and don't remove this check).

Uncaught Error: Assertion Failed: @computed can only be used on accessors or fields, attempted to use it with loadSession but that was a method. Try converting it to a getter (e.g. get loadSession() {})

@buschtoens
Copy link
Contributor

buschtoens commented Jun 3, 2019

The syntax above would only be unlocked with ember-concurrency-decorators. Standalone ember-concurrency can only be invoked as

class Foo {
  @(task(function*() {
    ...
  }).drop()) loadSession;
}

@robclancy
Copy link
Author

@buschtoens we have been over this many times. I am not going to use that terrible syntax. And when babel is fixed people are going to want to use the new syntax which this needs to support like everyone expects it does but noone has actually tried with the babel fix.

And ember-concurrency-decorators doesn't work with 3.10.

@buschtoens
Copy link
Contributor

And I have merely given you an explanation. What you have shown up there is not currently intended to work with standalone ember-concurrency and probably will not be until we were able to mature it in ember-concurrency-decorators.

The exact syntax you have shown will work with ember-concurrency-decorators, once the Babel fix officially lands and I get to integrate it with e-c-d, in case it is necessary.

You are more than welcome to send that PR yourself, if you want it before the official release of the Babel fix.

e-c-d might already work with the Babel fix in place using the syntax you want. Did you give it a try yet?

@robclancy
Copy link
Author

robclancy commented Jun 3, 2019

You have given a workaround, which everyone already uses. The current stuff with the babel fix would work with this package if the getter restriction is fixed according to this bug. And everyone already expects this babel fix will make it work like that, but it doesn't so here is a bug report.

Now you are ignoring more things. Read the bug report again. Read the last line of my comment again.

Did you give it a try yet?

... really?

@robclancy
Copy link
Author

I also love the passive aggressive you as if I'm the only one trying to use the normal syntax that they are fixing in babel and that you have posted the original bug report to get that syntax. So no not I only want it, you apparently do too.

@buschtoens
Copy link
Contributor

The current stuff with the babel fix would work with this package if the getter restriction is fixed according to this bug.

No, it would not. task doesn't even accept an options object. This is an ember-concurrency-decorators feature. This "getter restriction" is a result of ember-concurrency using computed, i.e. that assertion is coming from Ember core.

And everyone already expects this babel fix will make it work like that, but it doesn't so here is a bug report.

Via ember-concurrency-decorators.

Read the last line of my comment again.

OSS is a give and take. I did not mean that you test it in your app, but that you test your patched Babel version with e-c-d directly. Sorry for not being clear.

Any how, contrary to what I wrote in the README, the latest e-c-d version might still work with 3.10, as long as stage 1 decorators are enabled. I just did not explicitly test it for that, as I am only using Octane at my day job.

@robclancy
Copy link
Author

I know it is coming from the ember core, that is the point of why I say it extends it in the issue.

Why would this package not just support @task like what everyone expects? Part of upgrading to 3.10 is removing ember-concurrency-decorators like you remove other packages because it is expected to do things like everywhere else.

OSS is a give and take. I did not mean that you test it in your app, but that you test your patched Babel version with e-c-d directly. Sorry for not being clear.

3.10 does not work with e-c-d. How does it matter if it is our app or not?

You made an issue on babel to have decorators work properly with @task. They fixed it. The fix DOES NOT WORK WITH E-C OR E-C-D. And because this doesn't effect you directly you are here making excuses and telling me to use workarounds.

You know what, I'm just going to patch this myself like I did with babel since this isn't a bug apparently and then when people flood in here after the babel fix goes live expecting to be able to use the new syntax I'll know I told you so.

@buschtoens
Copy link
Contributor

FWIW ember-concurrency-decorators does work with Ember 3.10. I have added tests to assert this in machty/ember-concurrency-decorators#52 and updated the docs to reflect this in machty/ember-concurrency-decorators#53.

I also went ahead and tested the patched version of Babel with e-c-d and found that a minor change was required to make it work: machty/ember-concurrency-decorators#54

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

No branches or pull requests

2 participants