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

Convert elements to use ES6 modules (import/export) #6776

Merged
merged 4 commits into from
Nov 23, 2019

Conversation

etimberg
Copy link
Member

Converts the element files (src/elements/*) to use import/export.

import Point from './element.point';
import Rectangle from './element.rectangle';

export {
Copy link
Member Author

Choose a reason for hiding this comment

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

I used a non default export here since I think that will allow something like

import { Arc } from 'elements';

@etimberg
Copy link
Member Author

I started with the elements since I think the diff is minimal for now. The helpers require a lot more changes since we'd want to move toward exporting individual functions rather than exporting one large object.

benmccann
benmccann previously approved these changes Nov 21, 2019
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Great!
Just some wondering about style we should be using.

@@ -184,4 +184,4 @@ class Line extends Element {

Line.prototype._type = 'line';

module.exports = Line;
export default Line;
Copy link
Member

Choose a reason for hiding this comment

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

Should we use the

export default class Line extends ...

style instead?

Thinking about style consistency when we get to helpers:

export function helper1() {};
export function helper2() {};

vs

function helper1() {}
function helper2() {}

export {
  helper1
  helper2
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I started playing with the helpers files. Personally I preferred the first style but I'm not opposed to the 2nd if that's what everyone else prefers

Copy link
Member

Choose a reason for hiding this comment

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

I prefer brevity

@etimberg
Copy link
Member Author

Not sure why 1 test is failing. It says that it failed to create the chart, but I don't know why. Will debug later today

@kurkle
Copy link
Member

kurkle commented Nov 21, 2019

Chart namespace Chart.elements should be an object FAILED
Expected false to be truthy.

Not sure what instanceof those static import's are.

@etimberg
Copy link
Member Author

Yeah, I moved to a default export and it worked. Tests are still failing until #6780 is merged

kurkle
kurkle previously approved these changes Nov 22, 2019
@benmccann benmccann closed this Nov 23, 2019
@benmccann benmccann reopened this Nov 23, 2019
benmccann
benmccann previously approved these changes Nov 23, 2019
src/elements/index.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor

I closed and reopened the PR to kick off Travis again. The tests are passing now

@etimberg etimberg dismissed stale reviews from benmccann and kurkle via 1e8a386 November 23, 2019 12:23
@etimberg etimberg merged commit 62bbaeb into chartjs:master Nov 23, 2019
@etimberg etimberg deleted the element-exports branch November 23, 2019 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants