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

Autotracking Bugfix: Make the model property on the controller tracked #18074

Merged

Conversation

NullVoxPopuli
Copy link
Sponsor Contributor

@NullVoxPopuli NullVoxPopuli commented Jun 5, 2019

TODO:

  • write failing test
  • add tracked when auto-tracking is enabled

when the tracked flag is enabled, the controller's model property will be tracked by default.

@NullVoxPopuli NullVoxPopuli changed the title WIP: Make the model property on the controller tracked Autotracking Bugfix: Make the model property on the controller tracked Jun 5, 2019
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review June 5, 2019 21:37
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me.

@pzuraq - Any objections?

@NullVoxPopuli
Copy link
Sponsor Contributor Author

C.I. error:

unknown Statement of type "ClassDeclaration"

:(

@rwjblue
Copy link
Member

rwjblue commented Jun 6, 2019

I'm not sure exactly why this is an issue, but regenerator-runtime is throwing that error. Specifically, it doesn't like the inline class declarations within the test function.

There are two "easy" choices:

  • Rewrite to avoid using async / await -- This will almost certainly work, and should be fairly simple.
  • Rewrite class Foo extends Controller {} to let Foo = class Foo extends Controller {}-- I'm not 100% sure this will solve the issue, but it might.

@NullVoxPopuli
Copy link
Sponsor Contributor Author

Rewrite to avoid using async / await -- This will almost certainly work, and should be fairly simple.

is it worth configuring the tests to support async/await?

Copy link
Member

rwjblue commented Jun 7, 2019

They do support async / await in general, @pzuraq recently added that. The specific issue in this case is that (apparently) regenerator-runtime doesn't know what to do with a ClassDeclaration. The version in use, is older (0.11.1) so maybe it's fixed upstream already. I dunno.

@NullVoxPopuli NullVoxPopuli force-pushed the make-controller-model-tracked branch from 5105a48 to 9487bd7 Compare June 7, 2019 14:06
@rwjblue
Copy link
Member

rwjblue commented Jun 7, 2019

@NullVoxPopuli - Can you squash?

@NullVoxPopuli NullVoxPopuli force-pushed the make-controller-model-tracked branch from 9487bd7 to cd26304 Compare June 7, 2019 15:10
@NullVoxPopuli
Copy link
Sponsor Contributor Author

squashed

@NullVoxPopuli
Copy link
Sponsor Contributor Author

idk what's up with the git history here...

@NullVoxPopuli NullVoxPopuli force-pushed the make-controller-model-tracked branch from cd26304 to 58efd76 Compare June 7, 2019 16:47
CHANGELOG.md Outdated
@@ -16,6 +16,10 @@
- [#17940](https://github.com/emberjs/ember.js/pull/17940) [CLEANUP] Remove `sync` queue from @ember/runloop.
- [#18026](https://github.com/emberjs/ember.js/pull/18026) Enabling featured discussed in 2019-05-03 core team meeting.

### v3.10.1 (June 4, 2019)
Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did this come from O.o

@NullVoxPopuli
Copy link
Sponsor Contributor Author

@rwjblue fixed history ™️ :)

@NullVoxPopuli
Copy link
Sponsor Contributor Author

looks like browserstack had a problem

@NullVoxPopuli
Copy link
Sponsor Contributor Author

can someone click re-run?

@rwjblue rwjblue force-pushed the make-controller-model-tracked branch from 58efd76 to f2d49c7 Compare June 7, 2019 18:55
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.

4 participants