-
-
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
[Test]: fix for DataWrapper refactor in emberjs #7435
Conversation
Asset Size Report for ee8efc4 IE11 Builds EmberData increased by 53.0 B (-47.0 B compressed) which is within the allowed tolerance of 75 bytes uncompressedWarningsThe uncompressed size of the package @ember-data/store has increased by 41.0 B. Changeset
Full Asset Analysis (IE11)
Modern Builds EmberData increased by 53.0 B (-47.0 B compressed) which is within the allowed tolerance of 75 bytes uncompressedWarningsThe uncompressed size of the package @ember-data/store has increased by 41.0 B. Changeset
Full Asset Analysis (Modern)
Modern Builds (No Rollup) The size of the library EmberData has increased by 95.0 B (66.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/store has increased by 83.0 B. Changeset
Full Asset Analysis (Modern)
|
Performance Report for ee8efc4 Relationship Analysis
|
1bb9b38
to
7707e3a
Compare
6995288
to
fa6d521
Compare
fa6d521
to
03a2594
Compare
…r destroyed flag has flipped
… factoryManager is unable to have new properties set on it
package.json
Outdated
@@ -41,6 +41,7 @@ | |||
"@babel/plugin-transform-typescript": "^7.9.6", | |||
"@ember/edition-utils": "^1.2.0", | |||
"@ember/optional-features": "^1.3.0", | |||
"@ember/test-helpers": "^2.2.5", |
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.
Hmm, this won't work unless we also do the rest of the ember-qunit@5 updates.
packages/-ember-data/package.json
Outdated
"ember-cli-typescript": "^4.0.0", | ||
"ember-inflector": "^4.0.1" | ||
}, | ||
"devDependencies": { | ||
"@babel/plugin-transform-typescript": "^7.9.6", | ||
"@ember-data/unpublished-test-infra": "3.27.0-alpha.0", | ||
"@ember/optional-features": "^1.3.0", | ||
"@ember/test-helpers": "^2.2.5", |
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.
same here RE: ember-qunit@5
|
||
post.unloadRecord(); | ||
|
||
await settled(); | ||
|
||
assert.equal(removedIndex, 1, 'We are notified of the start index of a removal when we remove posts'); | ||
assert.equal(removedCount, 1, 'We are notified of the total posts removed'); | ||
if (gte('3.26.0')) { |
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.
Should probably add a comment about why we expect these to differ for 3.26+
packages/debug/package.json
Outdated
"ember-cli-test-info": "^1.0.0", | ||
"ember-cli-typescript": "^4.0.0" | ||
}, | ||
"devDependencies": { | ||
"@ember-data/unpublished-test-infra": "3.27.0-alpha.0", | ||
"@ember/optional-features": "^1.3.0", | ||
"@ember/test-helpers": "^2.2.5", |
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.
same here RE: ember-qunit@5
9febd5a
to
2981e7f
Compare
This reverts commit 4b74236.
@@ -9,6 +9,8 @@ module.exports = Object.assign({}, addonBaseConfig, { | |||
shouldRollupPrivate: true, | |||
externalDependenciesForPrivateModule() { | |||
return [ | |||
'ember', |
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.
Why did we have to add this?
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.
@@ -11,7 +11,7 @@ module('integration/store/creation-recursion', function(hooks) { | |||
let storeFactory = this.owner.factoryFor('service:store'); | |||
|
|||
this.owner.unregister('service:store'); | |||
this.owner.register('service:store', storeFactory); | |||
this.owner.register('service:store', storeFactory.class); |
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.
What caused this change?
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.
Ember asserts if you pass the factoryFor result directly to register now
@@ -437,7 +437,7 @@ export default class RecordDataDefault implements RelationshipRecordData { | |||
this._destroyRelationships(); | |||
this.reset(); | |||
if (!this._scheduledDestroy) { | |||
this._scheduledDestroy = run.backburner.schedule('destroy', this, '_cleanupOrphanedRecordDatas'); | |||
this._scheduledDestroy = emberBackburner.schedule('destroy', this, '_cleanupOrphanedRecordDatas'); |
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.
Does run complain now when you access backburner on it?
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.
Yes
@@ -6,6 +6,7 @@ import { registerWaiter } from '@ember/test'; | |||
import { DEBUG } from '@glimmer/env'; | |||
import Ember from 'ember'; | |||
|
|||
// TODO: expose Ember._Backburner as `import { _Backburner } from '@ember/runloop'` in ember-rfc176-data + emberjs/ember.js |
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.
Can you please clarify this a bit, not quite sure what the future work required is
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.
Ember doesn't expose the constructor for Backburner in a modules API
@@ -6,7 +6,7 @@ import { A } from '@ember/array'; | |||
import { assert } from '@ember/debug'; | |||
import { get, set } from '@ember/object'; | |||
import { assign } from '@ember/polyfills'; | |||
import { run as emberRunloop } from '@ember/runloop'; | |||
import { _backburner as emberBackburner } from '@ember/runloop'; |
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.
Why did we need this change?
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.
So its clearer across the board when we are working with Ember's backburner instances vs our internal
* Fix deprecations for accessing run.* (e.g. `run.next(...)`) * update to ember-cli-babel 7.26.3 And various other fixes for enabling ember-canary to pass Co-authored-by: Robert Jackson <[email protected]>
* Update Changelog for v3.26.0 * [Test]: fix for DataWrapper refactor in emberjs (#7435) * Fix deprecations for accessing run.* (e.g. `run.next(...)`) * update to ember-cli-babel 7.26.3 And various other fixes for enabling ember-canary to pass Co-authored-by: Robert Jackson <[email protected]> Co-authored-by: Robert Jackson <[email protected]>
https://github.com/emberjs/ember.js/pull/19379/files
close #7439