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

Model validation always fails when having duplicate classes #673

Open
dmitry-salnikov opened this issue Apr 20, 2018 · 3 comments
Open

Model validation always fails when having duplicate classes #673

dmitry-salnikov opened this issue Apr 20, 2018 · 3 comments

Comments

@dmitry-salnikov
Copy link

Astronomy doesn't handle the attempt to Class.create() with the name of already existing class.
Example: we have the next three files in project:

// file: MyModel.js
export const MyModel = Class.create({
  name: 'MyModel',
  fields: {
    foo: String
  }
});

// file: OtherModel.js
const MyModel = Class.create({
  name: 'MyModel',
  fields: {
    bar: String
  }
});

export const OtherModel = Class.create({
  name: 'MyModel',
  fields: {
    myModel: MyModel,
    default: function() { return new MyModel(); }
  }
});

// file: app.js
import { OtherModel } from 'OtherModel';

let otherModel = new OtherModel();
otherModel.myModel.bar = "123";
myModel.validate();

The validate() call on the last line will throw Error saying "myModel.bar has to be MyModel".

Looking at the Astronomy source /lib/core/class.js:140

let Class = this.classes[className] = class Class extends this {};

it's pretty clear that in such case the second declared class will simply overwrite the previous one.

Could you please take a look?

Thanks

@lukejagodzinski
Copy link
Member

lukejagodzinski commented Apr 20, 2018

One thing first, always check your examples before posting. You have several errors and first I had to figure out what you wanted to achieve, to actually start thinking about the answer to your problem. Don't give me extra work and always prepare reproduction repository. Preparing such a reproduction is a matter of a few minutes.

Why would anyone want to override class? If you do so, then you're probably doing something wrong. In the real-life application, no one would risk introducing two classes with the same name, because it's causing more confusion. So your problem can probably be solved just by renaming class.

However, after fixing your example everything works as expected so it's not a problem with Astronomy but with your code. Here is fixed code:

// MyModel.js
import { Class } from "meteor/jagi:astronomy";

export const MyModel = Class.create({
  name: "MyModel",
  fields: {
    foo: String
  }
});
// OtherModel.js
import { Class } from "meteor/jagi:astronomy";

const MyModel = Class.create({
  name: "MyModel",
  fields: {
    bar: String
  }
});

export const OtherModel = Class.create({
  name: "OtherModel",
  fields: {
    myModel: {
      type: MyModel,
      default() {
        return new MyModel();
      }
    }
  }
});
// app.js
import { MyModel } from "./MyModel"; // Just to demonstrate that it's being imported
import { OtherModel } from "./OtherModel";

const otherModel = new OtherModel();
otherModel.myModel.bar = "123";
otherModel.validate(); // No error thrown

You can also made this ugly check to make sure that indeed "otherModel.myModel.bar" requires string:

otherModel.constructor.schema.fields.myModel.type.class.schema.fields.bar.type.name // Returns "String"

@DarthRumata
Copy link

@lukejagodzinski I also have such issue. Of course I didn't have intent to override class with another class with same name. It was copy-paste mistake but your framework doesn't have check to prevent such kind of mistakes. It will be really good if you add it in next versions.

@lukejagodzinski
Copy link
Member

@DarthRumata indeed such a check would be a good idea to do just to prevent accidental overrides. I will fix that in the next release.

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

No branches or pull requests

3 participants