Skip to content

fix #9 and #13 - #18

Open
bung87 wants to merge 3 commits into
albinotonnina:masterfrom
bung87:master
Open

fix #9 and #13#18
bung87 wants to merge 3 commits into
albinotonnina:masterfrom
bung87:master

Conversation

@bung87

@bung87 bung87 commented Nov 25, 2019

Copy link
Copy Markdown

No description provided.

@albinotonnina

Copy link
Copy Markdown
Owner

Hey @bung87, thank you very much for the PR, CI is failing because of a test. Could you look into that?

@bung87

bung87 commented Nov 27, 2019

Copy link
Copy Markdown
Author

Hey @bung87, thank you very much for the PR, CI is failing because of a test. Could you look into that?

yeah , I fixed it in Fit container branch.

Comment thread lib/asset.js
constructor(name, pkg, options) {
super(name, pkg, options);
constructor(name, options) {
super(name, options);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could just super.apply(this, arguments) or super(...arguments) to future-proof this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yeah , can do this, better change arguments to other name like args, arguments is a keyword.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In this case, the use of the keyword is actually intended; we want to pass precisely the same arguments that were passed to us, to super(), so we do that with super(...arguments) (or the ES5 equivalent).

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.

3 participants