Update blueprints to use native JS classes

[ppcano]

Part of the Octane Tracking issue.

Atm, the blueprints will only generate Native Classes if the env EMBER_VERSION is equal to OCTANE; the octane-blueprints will set the variable.

It follows the same implementation than the Ember repo emberjs/ember.js#17621

[rwjblue]

LGTM, may need future tweaks to make this work better, but this lets us begin the iteration process....

[ppcano]

@rwjblue

I would like to get rid of the param { isModuleUnification: true }. This is not needed in the Ember blueprint. I think, we have to update the ember-test-helpers dependency and will work.

What are other changes came to your mind?

[rwjblue]

Not sure, I kinda want to get this in and start testing in apps to see what crops up...

[runspired]

Isn't this mixing syntaxes or am I misreading these in some way?

For instance, it looks like this might generate the following in some cases:

class Foo extends DS.Model {
  myAttr: DS.attr(),
  myBars: DS.hasMany('bar')
}

[ppcano]

@runspired

Isn't this mixing syntaxes or am I misreading these in some way?

😱 , Yes, you are right. I should have thought about the ED decorators case.

I have seen the ember-decorators is imported in the project. What is the current plan? A few options comes to my mind:

1. The Native Model Blueprint does not inject the attrs configuration, atm.

2. Implement attrs on the Native Model Blueprint using the @ember-decorators/data package.

3. Start the ED decorators for the attrs/haMany/belongsTo to be used later on this PR.

[runspired]

@pzuraq probably has the best insights for whether these blueprints should

1. be held off
2. use ember-decorators
3. if our own belongsTo/hasMany will "just work" with the right ember versions and we gate the blueprints on a min ember version

[pzuraq]

Happy to give context where I can!

1. We'll be creating a way to enable Octane via some sort of flag (currently it's an environment variable, but I believe they want to change that) and I believe Ember will be changing it's blueprints based on that flag (native classes if Octane, standard if not), so ideally Ember Data would also probably match that behavior. Unsure if this PR does that, if not it should hold off.
2. It should not need to use ember-decorators
3. The computed properties exported by Ember Data for attr, belongsTo, and hasMany should "just work" as decorators, no need for any updates I believe, though we should double check

[ppcano]

We could create a util function under lib/utilities to detect OCTANE. This function could be in the same module than lib/utilities/module-unification and this could be renamed to env-detector.js.

I would prefer to do this change in a follow-up PR and based on the reviewer instructions.

[ppcano]

@runspired

I think this PR is ready now; the Model blueprint can generate attr decorators and this behavior is validated on the tests.




pzuraq said:

should "just work" as decorators, no need for any updates I believe, though we should double check




I tested one case of the DS.attr decorator in a simple app and it worked.
 I thought that perhaps the easiest way to test the usage of the Ember Data hasMany, attr and belongsTo as decorators could be to change the existing tests like this one.

[ghost]

Tested this on a new app by running ember g model user firstName:string lastName:string age:number and found it generated the following:

import DS from 'ember-data';
const { Model } = DS;

export default class UserModel extends Model {
@DS.attr('string') firstName;
  @DS.attr('string') lastName;
  @DS.attr('number') age
}

Shouldn't the last attribute have a semicolon?

[ppcano]

@efx Thank you for testing and reporting. I have included the fix and it'll generate:

import DS from 'ember-data';
const { Model } = DS;

export default class UserModel extends Model {
  @DS.attr('string') firstName;
  @DS.attr('string') lastName;
  @DS.attr('number') age;
}

[runspired]

@ppcano in keeping with prettier and to ease the syntax a bit, could we make the output the following?

import DS from 'ember-data';
const { Model, attr } = DS;

export default class UserModel extends Model {
  @attr('string')
  firstName;
  @attr('string')
  lastName;
  @attr('number')
  age;
}

[ppcano]

@runspired Yes, I can work on these changes.

Just to double check:

@attr('string')
firstName;

instead of

@attr('string') firstName;

I lean toward the second.

[runspired]

@ppcano while I prefer the second, prettier formats the first. It doesn't make sense to me to give folks something that prettier would immediately invalidate.

[rwjblue]

prettier formats the first

Prettier has changed its stance on this, checkout this demo in the playground.

[runspired]

@rwjblue @ppcano awesome!

[ppcano]

@runspired The Model blueprint syntax has been improved. Now, it will generate:

// Octane apps
import DS from 'ember-data';
const { Model, attr } = DS;

export default class UserModel extends Model {
  @attr('string') firstName;
  @attr('string') lastName;
  @attr('number') age;
}

// Classic apps
import DS from 'ember-data';
const { Model, attr } = DS;

export default Model.extend({
  firstName: attr('string'),
  lastName: attr('string'),
  age: attr('number')
});

Let me know if Classic apps should continue using the DS.attr/DS.belongsTo/DS.hasMany/DS.Model syntax to remove that particular commit.

[runspired]

@ppcano this looks amazing! Is it in good shape for hasMany and belongsTo as well? If so, I think we are likely able to merge.

[ppcano]

@runspired Yes, hasMany and belongsTo are implemented and the tests also validate this behavior.