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.

Sign up for GitHub

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

Jump to bottom

[WIP] deprecate view registry #17804

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WIP] deprecate view registry #17804

wants to merge 2 commits into from

Conversation

chancancode
Copy link
Member

@chancancode chancancode commented Mar 26, 2019 •
edited
Loading

This fixes an issue where event dispatching stops working if id is passed with splattributes, because elementId will diverge from the actual id, causing the element to not be found during event dispatching.

This patch does not fix the elementId divergent problem, but changes the framework code to not rely on elementId and use a weak map instead. I have ideas for fixing the elementId issue, and making it optional, but I'll probably submit it separately. (It's fixed.)

It's probably best to review this commit-by-commit.

This also removes the view registry private API. The theory is that there are very few addons that uses it, and we can transition them to using the new Ember.ViewUtils.getElementView instead.

rwjblue
rwjblue reviewed Mar 26, 2019
View reviewed changes
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.

Generally 👍, only had one small question

packages/@ember/-internals/views/lib/system/utils.js Outdated Show resolved Hide resolved
@chancancode chancancode force-pushed the fix-event-dispatcher branch from 488078a to f79c3f3 Compare March 27, 2019 06:01
@chancancode chancancode changed the title Fix event dispatcher [WIP] Fix event dispatcher Mar 27, 2019
@chancancode chancancode force-pushed the fix-event-dispatcher branch from f79c3f3 to 0118483 Compare March 27, 2019 13:31
@chancancode chancancode changed the title [WIP] Fix event dispatcher Fix event dispatcher Mar 27, 2019
@chancancode chancancode requested a review from rwjblue March 27, 2019 13:41
@chancancode chancancode force-pushed the fix-event-dispatcher branch 6 times, most recently from 9316218 to e00465f Compare March 28, 2019 03:13
@chancancode chancancode force-pushed the fix-event-dispatcher branch from e00465f to 3474c44 Compare March 28, 2019 03:45
@chancancode chancancode requested a review from wycats March 28, 2019 03:46
@chancancode chancancode mentioned this pull request Mar 28, 2019
Merged
5 tasks
rwjblue
rwjblue approved these changes Mar 28, 2019
View reviewed changes
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.

LGTM

@chancancode chancancode mentioned this pull request Mar 29, 2019
Merged
@chancancode chancancode force-pushed the fix-event-dispatcher branch from 3474c44 to 85a34c0 Compare March 29, 2019 02:48
@chancancode
[CLEANUP] Remove ViewRegistry
90e8079 
The framework does not need it anymore.

In most cases, `Ember.ViewUtils.getElementView` can be used as a
replacement to get the classic component (`Ember.Component`) that
corresponds to a DOM element (if any).
@chancancode
[BUGFIX canary] Ensure elementId matches reality
172849e 
Since the `id` attribute can be passed through splattributes, we cannot
assume the generated `elementId` will be used. Instead of eagerly generating
an `elementId` (if not set in `init`), we make it as lazy as possible and
read from DOM after flushing the attributes. This ensures the value reflects
what was eventually set. If, however, an `elementId` is explicitly set during
`init`, we want to ensure they match up when `elementId` is accessed again.

Note that with this commit, the framework no longer rely on `elementId` for
any purpose. We may want to deprecate _reading_ from `elementId`, making it
a "write-only" API that only serve to customize the wrapper element.
@chancancode chancancode force-pushed the fix-event-dispatcher branch from 85a34c0 to 172849e Compare March 29, 2019 04:48
@mixonic
Copy link
Member

mixonic commented Mar 29, 2019

Should -view-registry go through an intimate API deprecation re: usages in the wild?

@chancancode chancancode reopened this Mar 29, 2019
@chancancode chancancode changed the title Fix event dispatcher [WIP] deprecate view registry Mar 29, 2019
@rwjblue
Copy link
Member

rwjblue commented Apr 1, 2019

Should -view-registry go through an intimate API deprecation re: usages in the wild?

I did review usages on EmberObserver.com and I do not think the limited usage merits intimate API deprecation. The usages in addons that are maintained (releases in the last year or so) are mostly official packages (@ember/test-helpers, ember-resolver in the MU flagged code, etc), the usages in maintained non-official addons seem to mostly be in custom testing utilities and actually do have a nice API to migrate to.

@rwjblue
Copy link
Member

rwjblue commented Feb 10, 2021

@chancancode - Should we close, or revive?

@chancancode
Copy link
Member Author

huh yes, I thought we did the elementId fix already.. did we not?

@chancancode
Copy link
Member Author

chancancode commented Feb 10, 2021 •
edited
Loading

Ah we did in fact land #17818 at least. We can probably just remove this now since the inspector switched to using the debug render tree. Though I just added a test in #19382 that uses the view registry because I was lazy. 😉

@locks
Copy link
Contributor

locks commented Feb 5, 2022

@chancancode is this still relevant?

@kategengler
Copy link
Member

Is this still relevant? It is labeled wip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

@wycats wycats wycats approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Triaging
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chancancode @mixonic @rwjblue @locks @kategengler @wycats