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

Racer 2 #302

Merged
merged 152 commits into from
May 1, 2024
Merged

Racer 2 #302

merged 152 commits into from
May 1, 2024

Conversation

craigbeck
Copy link
Contributor

@craigbeck craigbeck commented Apr 9, 2024

racer@2 breaking changes:

  • The Model class should no longer be directly constructed. Use createModel() instead.
    • For example, new racer.Model() -> racer.createModel()
    • Directly trying to construct Model can result in errors like Cannot set properties of undefined (setting 'socket') from Model.createConnection
  • racer no longer and instance. racer now a proper namespace
  • legacy event handlers not supported by typescript
  • Drop support for node 10-14
  • remove deprecated getModel() method form req object in modelMiddleware

Copy link
Contributor

@ericyhwang ericyhwang left a comment

Choose a reason for hiding this comment

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

Initial comments on several files, still a bunch more to go through

Comment on lines +396 to +398
// (value, null)
value = arguments[0];
cb = arguments[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there cases where null is passed as a callback explicitly?

@@ -167,7 +333,7 @@ Model.prototype._create = function(segments, value, cb) {
var event = new ChangeEvent(value, previous, model._pass);
model._emitMutation(segments, event);
}
this._mutate(segments, create, cb);
return this._mutate(segments, create, cb);
Copy link
Contributor

Choose a reason for hiding this comment

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

The typing of model.create() indicates that it doesn't return anything (void return), so a technically more accurate thing to do would be:

  1. Remove the return above in return this._create(segments, value, cb);
  2. Then this new extra return here isn't needed
  3. Same in _createNull, the new return is no longer needed either

Comment on lines 84 to 85
pushPromised(value: any): Promise<number>;
pushPromised(subpath: Path, value: any): Promise<number>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think so, since callback doesn't get invoked with any result

src/Model/Doc.ts Outdated
Comment on lines 2 to 8
import { Collection } from './collections';
import { Path } from '../types';

function Doc(model, collectionName, id) {
this.collectionName = collectionName;
this.id = id;
this.collectionData = model && model.data[collectionName];
}
export class Doc {
collectionData: Model;
collectionName: string;
data: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

data only appears to be used by LocalDoc, not RemoteDoc. Might be possible to move it to the LocalDoc subclass, instead of having it here in the base class

if (segments && segments.length) path += '.' + segments.join('.');
return path;
};
constructor(model: Model, collectionName: string, id: string, data?: any, _collection?: Collection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably remove the final unused _collection param

Copy link
Contributor Author

@craigbeck craigbeck Apr 26, 2024

Choose a reason for hiding this comment

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

Used here https://github.com/derbyjs/racer/blob/racer-2/src/Model/collections.ts#L211

But cant see why this is passed 🤷

... oh.. used on RemoteDoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing it leads to type errors where we pass it, then removing that leads to test errors. Looks like a rabbit hole for later

}

export class RootModel extends Model<ModelData> {
backend: RacerBackend;
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically should be optional, as it's only present on server-side models created with backend.createModel(), but may cause type breakages if made optional. Could try it and see

src/index.ts Outdated
Comment on lines 39 to 41
export function createBackend(options?: BackendOptions) {
return new RacerBackend(racer, options);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, it might cause a bundler to bring in the backend code. We could change this to a stub implementation that throws, and switch the RacerBackend to a type import for type info

@craigbeck craigbeck merged commit 168d03d into master May 1, 2024
8 checks passed
@craigbeck craigbeck deleted the racer-2 branch May 1, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants