Fix #30 by using a for loop instead of .forEach

[themasch]

iterate over routes using a for loop instead of .forEach so we can iterate over objects as well

I figured I might as well create a PR for this issue. afaik this should be the "cleanest" solution for this problem the inner body does not care about the routes name anyway.

It hurts me a little bit to remove functional syntax and the fat arrow, but I guess its for the greater good ;)

An alternative would be to use Object.values(routing.routes).forEach but that would give no access to the route name at all. With the for loop theres still the key if one needs the name.

Additionally I guess this is faster than Object.values.

[CLAassistant]

All committers have signed the CLA.

[coveralls]

Pull Request Test Coverage Report for Build 175

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 62.864%

---

Totals
Change from base Build 167:	0.0%
Covered Lines:	425
Relevant Lines:	620

---

💛 - Coveralls

[codeworrior]

Basically LGTM, but we decided for the same commit message rules as in openui5, so please prefix your commit message with e.g. [FIX]

[RandomByte]

Just making sure as mdn notes:

for...in should not be used to iterate over an Array where the index order is important.

The order of routes is and will not be relevant here?

[codeworrior]

Yes, order of routes is currently not of any interest for the dependency analysis.

[matz3]

I would prefer a more explicit handling of both types as it might be hard to understand for others. At least I was at first thinking that only objects are supported now. But if I'm the only one, I don't mind 😄

[themasch]

@codeworrior oh sure. I'll edit the commit message and force push.

@matz3 I could do something like

if (Array.isArray(routing.routes)) {
  routing.routes.forEach(this.visitRoute.bind(this, info))
} else {
  for ( let key in routing.routes) {
    this.visitRoute(info, routing.routes[key])
  }
}

and move the loop body to a method (visitRoute in this example).

That way the handling of arrays would not change fromt he current version and objects would be handled like in this pr.

Another way would be to use Object.values(routing.routes).forEach(…).

[matz3]

Looks good to me 👍
@codeworrior @RandomByte what's your opinion?

[RandomByte]

routing.routes.forEach(this.visitRoute.bind(this, info))

While this is pretty cool once understood, I think for the casual reader it involves too much magic (I had to look this up tbh) 😄

What about a good old arrow function?

if (Array.isArray(routing.routes)) {
  routing.routes.forEach((route) => this.visitRoute(info, route));
} else {
  for ( let key in routing.routes) {
    this.visitRoute(info, routing.routes[key]);
  }
}

[themasch]

Oh sure, 👍 for the arrow function :D

Should I restructure the PR to implement it that way?

[RandomByte]

Should I restructure the PR to implement it that way?

Yes please. I think @codeworrior will also be fine with that 🙂

[themasch]

As the rework got a a little big to do without a safety net I added two test cases.

While writing this I noticed some other thing the current code might not handle correctly:

• route.target might be an array of target-keys (see Route docs)
• route.viewPath and route.viewType may override routing.config.view{Path,Type}.

The current implementation ignores both cases.

I could add a commit to this PR to fix those cases (+test) or do this in another PR (or forget I ever thought about it, if you want me to ;))

[codeworrior]

I had fixed those two issues in a branch of our internal predecessor repo already, but that branch didn't make it into the public repos.

That's how I fixed the second:

	// merge target config with default config
	let config = Object.assign({}, routing.config, target);
	let module = ModuleName.fromUI5LegacyName(
		(config.viewPath ? config.viewPath + "." : "") +
		config.viewName, ".view." + config.viewType.toLowerCase() );

If you don't mind, I can push the two fixes on top of your change. It might take some time until I can migrate the old branch to the ui5-builder repo.