lukejacksonn / ijk

Transforms arrays into virtual dom trees; a terse alternative to JSX and h
MIT License
466 stars 17 forks source link

Optimise clean function perf #4

Closed crazy4groovy closed 6 years ago

crazy4groovy commented 6 years ago

Cool lib!

What about

const clense = (a, b) => {
  b && a.push.apply(a, b)
  return a
}

or maybe?

const clense = (a, b) => {
  b && a.push.apply(a, [b])
  return a
}

to save some memory via mutating?

lukejacksonn commented 6 years ago

Hey, thanks!

I hadn't looked into perf too much yet but this makes sense to me and as good of a place to start as any. So three questions:

Mytrill commented 6 years ago

Maybe at node[2].reduce(clense, []).map(build(x, y, z)) the build(x, y, z) could be cached? Not sure if that's possible though..

crazy4groovy commented 6 years ago

Sounds like a good time to first put tests in place ;)

okwolf commented 6 years ago

@crazy4groovy if you are talking about simple unit tests for functional regression testing you're welcome to use my own library Tead.

For performance testing, I have less knowledge regarding which tools to use.

lukejacksonn commented 6 years ago

Hahaa, I am a small way through implementing these.. in Tead like @okwolf suggested! There is only a simple set to start off with. I will push them up after work this evening.

okwolf commented 6 years ago

@lukejacksonn welcome to the (somewhat exclusive) Tead club! Let me know how your experience is using it, and what you'd like improved.

lukejacksonn commented 6 years ago

@okwolf https://github.com/lukejacksonn/ijk/pull/5

lukejacksonn commented 6 years ago

I have added tests to the repo 🔬 which means you can now play in (relative) safety 🎉

I tried running the code you provided @crazy4groovy and it doesn't pass the tests. I thinks the problem is it doesn't supporting the unwrapping of arrays in children arrays; like this:

[ 'main', [
  'Hello World',
  [['li', 1],['li', 2],['li', 3]],
]]

Which makes returning a map possible without having to spread it into the parents children array:

const Item = x => ['li', x]
['main', [
  'Hello World',
  [1,2,3].map(Item),
]]

That could be fixed with some tweaking but again, it is hard to know the actual benefits until we write some perf tests for it too. This can't be too hard but I have never actually done it so might take some time.

Advice welcomed!

lukejacksonn commented 6 years ago

Fixed by #17 thanks @crazy4groovy