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

Report for 'Constant string in component HTML attributes #478'. New t… #563

Merged

Conversation

jccazeaux
Copy link
Contributor

…ests for components reveal an error in forEach on attributes: attributes is not an array. An error was revealed too in View import : cyclic dependency between view and binding;

Two important notes on this report

  1. Import of View from module view.js couldn't be resolved because view.js already used bindings.js in his imports. To break this cycle i replace new View by rivets.bind() which provides exactly the same result.
  2. I introduced all components tests given by @blikblum. This revealed a bug due to use of forEach on element attributes. attributes is not an array and can not be browsed with forEach.

…. New tests for components reveal an error in forEach on attributes: attributes is not an array. An error was revealed too in View import : cyclic dependency between view and binding;
@blikblum
Copy link
Contributor

blikblum commented Feb 2, 2016

You can use Array.prototype.slice.call(this.el.attributes).forEach(attribute => ...)
or
Array.prototype.forEach.call(this.el.attributes, attribute => ...)

@jccazeaux
Copy link
Contributor Author

Yes i'm a bit old school on loops ;)
But as for() loops are faster than forEach i think it's not that bad.

@@ -278,18 +278,23 @@ export class ComponentBinding extends Binding {
let bindingRegExp = view.bindingRegExp()

if (this.el.attributes) {
this.el.attributes.forEach(attribute => {
let attribute = null
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you create it here? I believe you can remove this line and move let on 283

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an old java reflex. I never declare variables in loops. I know in javascript it isn't as important but sometimes i can't help it ;)

@benadamstyles benadamstyles merged commit 6328793 into mikeric:es6 Aug 9, 2016
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.

None yet

4 participants