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

Fix property access in production and turn on prod testing #1376

Merged
merged 2 commits into from Sep 29, 2021
Merged

Conversation

igorT
Copy link
Collaborator

@igorT igorT commented Sep 28, 2021

Fixes #1371

@github-actions
Copy link

github-actions bot commented Sep 28, 2021

Performance Report for 38045f2

Scenario - materializing-lots: ☑️ Performance is stable

☑️ duration
phase no difference [-13ms to 135ms]
☑️ Phase [navigationStart] => [start-loading]
phase no difference [-4ms to 8ms]
⚠️ Phase [start-loading] => [pushed-payload]
phase estimated regression +80ms [44ms to 120ms] OR +1.75% [0.95% to 2.61%]
☑️ Phase [pushed-payload] => [end-loading]
phase no difference [-56ms to 24ms]
☑️ Phase [end-loading] => [Test End]
phase no difference [0ms to 2ms]

Scenario - materializing-small: ☑️ Performance is stable

☑️ duration
phase no difference [-6ms to 8ms]
☑️ Phase [navigationStart] => [start-loading]
phase no difference [-2ms to 7ms]
☑️ Phase [start-loading] => [pushed-payload]
phase no difference [-3ms to 0ms]
☑️ Phase [pushed-payload] => [end-loading]
phase no difference [-2ms to 1ms]
☑️ Phase [end-loading] => [Test End]
phase no difference [-1ms to 1ms]

Scenario - rendering: ☑️ Performance is stable

☑️ duration
phase no difference [-24ms to 21ms]
☑️ Phase [navigationStart] => [start-loading]
phase no difference [-8ms to 3ms]
☑️ Phase [start-loading] => [pushed-payload]
phase no difference [-4ms to 0ms]
☑️ Phase [pushed-payload] => [end-loading]
phase no difference [-1ms to 1ms]
☑️ Phase [end-loading] => [Test End]
phase no difference [-15ms to 22ms]

Copy link
Collaborator

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

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

LGTM, only real complaint is let's update the comment in the proxy as the code is a little tricky and it depends on particular details of Ember.get

tests/unit/model/native-access-test.js Show resolved Hide resolved
@igorT igorT requested a review from hjdivad September 29, 2021 22:00
Copy link
Collaborator

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

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

LGTM please combine the commits before merging so we have clean commit history

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

Successfully merging this pull request may close these issues.

Native property access doesn’t work in production
2 participants