-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: Native Model #7226
refactor: Native Model #7226
Conversation
cc @snewcomer if we can land this then we can probably do some nice things to make sure that folks calling |
Asset Size Report for 7c95440 IE11 Builds 🛑 The size of the library EmberData has increased by +3.34 KB (+354.0 B compressed) which exceeds the failure threshold of 75 bytes.Warnings
Changeset
Full Asset Analysis (IE11)
Modern Builds 🛑 The size of the library EmberData has increased by +2.41 KB (+401.0 B compressed) which exceeds the failure threshold of 75 bytes.Warnings
Changeset
Full Asset Analysis (Modern)
Modern Builds (No Rollup) 🛑 The size of the library EmberData has increased by +1.75 KB (+214.0 B compressed) which exceeds the failure threshold of 75 bytes.Warnings
Changeset
Full Asset Analysis (Modern)
|
Performance Report for b44fd1d Relationship Analysis
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a phenomenal refactor. A thought on the isError
test error but not sure of a solution. Do you think we need the setter?
@runspired can we merge this? |
b37b67f
to
75f60fd
Compare
ef4/ember-data-relationship-tracker#19 Ok final one to get these tests to pass. |
75f60fd
to
cf641c2
Compare
f90f6ae
to
7f48662
Compare
27fd9c2
to
0f4e977
Compare
4c20fc4
to
a975aba
Compare
a975aba
to
7ce46c1
Compare
7ce46c1
to
b44fd1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Audited and it seems all of @igorT's feedback was addressed.
dadb84e
to
88d3550
Compare
isNewCP = retrieveFromCurrentState; | ||
} | ||
function computeOnce(target, key, desc) { | ||
const cache = new WeakMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left comments
Co-Authored-By: Scott Newcomer <[email protected]> fix docs tests getWithDefault bad rebase fix doc tests add computeds dont invalidate prop within isError dont install tracking within our meta object more prop cleanup more get cleanup cycle less on id cycle less on id cleanup update test Revert "cycle less on id cleanup" This reverts commit 173c42b. Revert "cycle less on id" This reverts commit 38f76aa. less cycling but without whatever weird build issue remove mixin if deprecation is resolved and strip deprecations for perf tests new tests for refactoring refactor to simpler definitions
adb0b01
to
4700c60
Compare
Now that our minimum Ember LTS is
3.12 (and for current canary 3.16! although we didn't go that far here)🎉 3.20 🎉we can refactor Model to be a native class. (support for this began in 3.6 and decorator support began in 3.10)Note:
in 3.16 minimum supportwe can embrace a @Tracked world, we should prepare :) (this also helps us prepare)This PR does that refactor as minimally as possible. The few test changes were minor cleanups I did while investigating failures (not fixes, just made debugging easier) that seemed worthwhile to just keep.