-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
d12c38c
to
cc16596
Compare
This commit is relatively simple: - move chunks of code from backbone.js to new files - apply eslint + prettier changes (100% automated) The goal of this commit is to make especially obvious and plain what we're actually changing in subsequent commits.
BREAKING CHANGE: loss of support for `History`, `Router` and `View`.
0351e85
to
879210c
Compare
This helps make future commits more comprehensible in their refactor.
8de8849
to
dbc8411
Compare
if (!model.validationError) return model; | ||
this.trigger('invalid', this, model.validationError, { | ||
...options, | ||
validationError: model.validationError, |
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 functionality was previously provided by an undocumented side-effect of the Model
constructor.
src/Collection.js
Outdated
_.extend(Collection.prototype, Events, { | ||
// The default model for a collection is just a **Backbone.Model**. | ||
// This should be overridden in most cases. | ||
model: Model, |
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.
Prototype-inherited fields like these end up tacked on after the class
definition.
mixinMethods( | ||
Collection, | ||
[ | ||
['chain', _.chain, 1], |
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.
Methods by reference instead of name so we can appropriately import them from lodash
at compile time and trim the build accordingly. Still unfortunate that a lot of Backbone's value is by mixing in these functions.
export const Model = /*#__PURE__*/ (() => { | ||
// eslint-disable-next-line no-shadow | ||
const Model = class Model { | ||
static extend = extend; |
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.
Compat. Might be able to remove this in a future version.
// If you specify an attribute name, determine if that attribute has changed. | ||
hasChanged(attr) { | ||
if (attr == null) return !_.isEmpty(this.changed); | ||
return _.has(this.changed, [attr]); |
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.
For exact parity with backbone
(via underscore), put the attribute in an array so it isn't interpreted as a dot-delimited path.
// receive the true name of the event as the first argument). | ||
Events.trigger = function (name) { | ||
if (!this._events) return this; | ||
const ids = obj ? [obj._listenId] : keysIn(listeningTo); |
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.
Note that with regenerator, we end up with this getting deoptimized back to the equivalent of Object.keys
.
child = function () { | ||
return parent.apply(this, arguments); | ||
}; | ||
child = class extends parent {}; |
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.
Note that the class
syntax automatically configures the prototype
, including the prototype.constructor
field.
parsed.append(key, value); | ||
} | ||
return url.slice(0, queryIndex + 1) + parsed.toString(); | ||
} |
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 function (stringifyGETParams
) is notably different from upstream.
export function setEmulateHTTP(value) { | ||
emulateHTTP = value; | ||
} | ||
|
||
export function setEmulateJSON(value) { | ||
emulateJSON = value; | ||
} |
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.
Gotta make these setters - mostly for the tests, but also for use downstream.
for (i = 0; i < tail.length; i++) tail[i] = array[i + at]; | ||
for (i = 0; i < length; i++) array[i + at] = insert[i]; | ||
for (i = 0; i < tail.length; i++) array[i + length + at] = tail[i]; | ||
} |
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.
Probably able to use Array#splice
instead, or at least optimize to remove the need for an additional tail
array.
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.
According to jashkenas#4027 (comment), splice
is sloooow.
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.
Ah I misread the comment - splice
in a loop is slow, which is 100% accurate because then you're repeatedly copying the tail. Much better to bailout to a manual splice insertion implementation when there are too many items.
Co-authored-by: Shil Sinha <shil.sinha@gmail.com>
Co-authored-by: Shil Sinha <shil.sinha@gmail.com>
see https://github.com/mixmaxhq/boddle/blob/initial-release/README.md
honestly the more I type it the more I dislike the name
review link