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

[REFACTOR] Removes most of the remaining custom reference code. #239

Merged
merged 1 commit into from
Feb 14, 2020

Conversation

pzuraq
Copy link
Member

@pzuraq pzuraq commented Feb 14, 2020

Removes most of the remaining custom reference code, with the exception
of helper references (which will be factored out soon).

Also removes the tracking/rendering assertion for the time being,
pending discussion on whether or not the assertion continues to make
sense. If the discussion concludes it does, we can add it back before
the v2 release, with more thorough integration testing that actually
tests the expected behavior, rather than unit tests.

Removes most of the remaining custom reference code, with the exception
of helper references (which will be factored out soon).

Also removes the tracking/rendering assertion for the time being,
pending discussion on whether or not the assertion continues to make
sense. If the discussion concludes it does, we can add it back before
the v2 release, with more thorough integration testing that actually
tests the expected behavior, rather than unit tests.
@pzuraq pzuraq force-pushed the refactor/remove-custom-references branch from 4da63d0 to eb66ed8 Compare February 14, 2020 03:02
@pzuraq pzuraq merged commit 421500e into master Feb 14, 2020
@pzuraq pzuraq deleted the refactor/remove-custom-references branch February 14, 2020 03:25
@chadhietala
Copy link
Member

I think we added that assertion because it wasn't really obvious when working on an app that a field wasn't being tracked.

@pzuraq
Copy link
Member Author

pzuraq commented Feb 18, 2020

The issue is that with autotracking, we can no longer provide guarantees about this assertion. Basically, as soon as you add a getter, we can’t wrap any properties that getter accesses.

Personally, I think this is worse DX. A dev might encounter the error the first time with a normal property, think “Oh, Glimmer will tell me if I need to add the decorator, neat!”, and then be confused and upset when that doesn’t work in all cases.

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

Successfully merging this pull request may close these issues.

2 participants