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

[RFR] Starting migration to ES6 #330

Merged
merged 20 commits into from
Mar 10, 2015
Merged

[RFR] Starting migration to ES6 #330

merged 20 commits into from
Mar 10, 2015

Conversation

jpetitcolas
Copy link
Contributor

This PR aims to introduce ES6 features on ng-admin. For a first PR, we will restrict to Main/component/service/config folder.

Retro-compatibility should be preserved, still using getters and setters the functional way (field.validation({ required: true })).

Left tasks:

  • Fix creation view
  • Fix show view
  • Add detail links on list views
  • Fix list filters
  • Use core-js polyfill
  • Fix tests
  • Review build process to concatenate polyfill with configuration
  • Add back fields setter behavior, calling it several times should append fields instead of replacing all of them
  • Add mocha tests to grunt

@jpetitcolas
Copy link
Contributor Author

I finally get a display on dashboard view, using the new ES6 classes.

I'm wondering if we shouldn't break retro-compatibility with this major change. Indeed, to make it compliant with the old version, I had to remove getters and setters (to prevent name conflict issues) to put function like:

layout() {
    if (arguments.length) {
        this._layout = arguments[0];
        return this;
    }

    return this._layout;
}

Moreover, I am not happy with the way I render classes publicly accessible, using:

window.ngadmin = window.ngadmin || {};
window.ngadmin.Application = Application;

But I didn't find a cleaner solution.

Still in (lot of) WIP.

@jpetitcolas
Copy link
Contributor Author

@fzaninotto, I spotted something weird with master blog example config:

comment.editionView()
    .fields(comment.creationView().fields())
    .fields([
        nga.field(null, 'template')
            .label('')
            .template('<post-link entry="entry"></post-link>')
    ]);

We call the fields method twice. Wouldn't it be more intuitive to consider fields as a more common setter, just replacing fields value when called instead of appending it?

I propose to replace it by:

comment.editionView().fields([
    comment.creationView().fields(),
    nga.field(null, 'template')
        .label('')
        .template('<post-link entry="entry"></post-link>')
]);

Do you agree? It would break a little bit BC, but not sure a lot of people use this syntax. Of course it will be mentioned in the changelog;

@fzaninotto
Copy link
Member

You're right that fields() should behave like a setter and not an adder, except that your second example also shows something unexpected: adding an array of fields instead of a field. As I don't want to pass up a sure thing, until we find a better way to reuse existing fields, I suggest we stay as is for now.

@jpetitcolas
Copy link
Contributor Author

We already pass both fields and array to fields method. For instance, the post.editionView configuration:

post.editionView()
    // ...
    .fields([
        post.creationView().fields(),
        nga.field('tags', 'reference_many')
            .targetEntity(tag)
            .targetField(nga.field('name'))
            // ...
    ]);

This is why we have to flatten the fields array at some places.

I propose to add a console.warning if using fields() setter when having already some fields, and switch to the new behavior at next version.

Warning: you are using fields setter while there is already some configured fields. In next version, calling several times this method will replace all fields, instead of just appending them. You should consider using fields(existingFieldsArray, newFieldA, newFieldB, ...) instead.

What do you think?

@jpetitcolas
Copy link
Contributor Author

I updated PR description with left tasks.

dest: 'examples/blog/build/',
expand: true
},
corejs: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need as already embedded in ng-admin-configuration.js

@jpetitcolas jpetitcolas changed the title [WIP] Starting migration to ES6 [RFR] Starting migration to ES6 Mar 5, 2015
@jpetitcolas
Copy link
Contributor Author

Switching to RFR.

@jpetitcolas
Copy link
Contributor Author

All tests are passing locally. Travis fails because of SauceLabs. I let you review it, and then I'll push my changes to re-trigger another build.


## Configuration factory

Configuration is now done through a `ConfigurationFactory`. You can retrieve it directly from Angular DI. You just have to retrieve your application from this factory instead of the `NgAdminConfigurationProvider` as previously:
Copy link
Member

Choose a reason for hiding this comment

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

no trace of ConfigurationFactory in the code below.

@fzaninotto
Copy link
Member

Please check out https://github.com/marmelab/ng-admin/pull/327/files and the current master version of custom Fields to make sure you don't forget anything.

super(name);
this._type = "number";
}
}
Copy link
Member

Choose a reason for hiding this comment

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

don't forget fractionSize()

@jpetitcolas jpetitcolas force-pushed the es6 branch 2 times, most recently from 08db795 to 984398d Compare March 10, 2015 10:37
fzaninotto added a commit that referenced this pull request Mar 10, 2015
[RFR] Starting migration to ES6
@fzaninotto fzaninotto merged commit b436598 into master Mar 10, 2015
@fzaninotto fzaninotto deleted the es6 branch March 10, 2015 12:22
@fzaninotto
Copy link
Member

Thanks a lot!

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