-
Notifications
You must be signed in to change notification settings - Fork 2
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
Defactor: remove TypeScript, native classes / decorators, ember-concurrency #4
Conversation
return () => | ||
get(this, 'isDestroyed') || | ||
get(this, 'isDestroying') || | ||
get(this, '_threadId') !== threadId; |
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.
@buschtoens I don't understand this line, because it's always will be equal
{} !=== {}
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.
Talked about this on Discord, but for anyone following along at home and also wondering:
Previously loadEngine
was a restartable ember-concurrency task, meaning that whenever a new instance is started, any old potentially still running instance is cancelled. This is to make sure that, we don't accidentally render stale content, because of async calls, when the input arguments change.
The trick here with {}
is:
{} === {} // => false
const obj = {};
obj === obj // => true
Whenever loadEngine
is called it starts a new "thread", which then sets this._threadId
to a new object. So any old still running threads would then compare there own closure threadId
to the new this._threadId
, which is then unequal.
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.
sound good, thank you bud
This PR removes all non-essential niceties to reduce bundle size for users that don't use
ember-concurrency
andember-decorators
. It also reverts TypeScript back to the oldschool Ember Object Model to enable merging withember-engines
/ember-asset-loader
/ Ember core.Fixes #3. Most likely also fixes #2.
/cc @villander @dgeb