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

WIP es2015, rollup config file, and source maps #73

Merged
merged 4 commits into from
Mar 20, 2017

Conversation

micahstubbs
Copy link
Collaborator

@micahstubbs micahstubbs commented Mar 20, 2017

This PR shows what an es2015 version of d3-component could look like.

new is a rollup.config.js file for easier rollup configuration, as well as source maps.

still to do:

@curran
Copy link
Owner

curran commented Mar 20, 2017

Woohoo! Thanks @micahstubbs this looks awesome. Will take a closer look.

Copy link
Owner

@curran curran left a comment

Choose a reason for hiding this comment

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

Thanks a ton @micahstubbs ! This ES2015ification really cleans things up.

Will verify tests locally & fix the setters for chainability.

@@ -16,7 +16,7 @@
"url": "https://github.com/curran/d3-component.git"
},
"scripts": {
"pretest": "rm -rf build && mkdir build && rollup -f umd -g d3-selection:d3 -n d3 -o build/d3-component.js -- index.js",
"pretest": "rm -rf build && mkdir build && rollup -c",
Copy link
Owner

Choose a reason for hiding this comment

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

Nice.

"uglify-js": "2"
"uglify-js": "2",
"eslint": "^2.9.0",
"eslint-config-airbnb": "^9.0.1",
Copy link
Owner

Choose a reason for hiding this comment

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

Awesome. I'm all for it - ESLint & Babel.

let pkg = require('./package.json');
let external = Object.keys(pkg.dependencies);

export default {
Copy link
Owner

Choose a reason for hiding this comment

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

A Good Thing, moving all this to a config file.

render = noop,
destroy = noop,
key;
const setInstance = (node, value) => { node.__instance__ = value; }; // no operation
Copy link
Owner

Choose a reason for hiding this comment

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

Oh wow, this stuff cleans up nicely with arrows.

instance && instance.destroy();
function destroyDescendant() {
const instance = getInstance(this);
if (instance) { instance.destroy(); }
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah this is definitely more clear.

component.create = function(_) { return (create = _, component); };
component.destroy = function(_) { return (destroy = _, component); };
component.key = function(_) { return (key = _, component); };
component.render = (_) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm guessing this change was ESLint-induced. I'll iterate on this part a bit more. These setters should return component for chainability, like this:

component.create = (_) => {
  create = _;
  return component;
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@curran please do improve this. I actually manually refactored this, in response to an eslint error. I wasn't certain of the return value, and so took a guess.

the rule it complained about here is http://eslint.org/docs/rules/no-return-assign

@curran curran mentioned this pull request Mar 20, 2017
@curran curran merged commit 6122003 into curran:master Mar 20, 2017
@curran curran mentioned this pull request Mar 20, 2017
@micahstubbs micahstubbs deleted the es2015-rollup branch March 20, 2017 13:08
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