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

Why is the tree reduced instead of mapped in handleExtendsNodes? #26

Open
HoldYourWaffle opened this issue Apr 19, 2019 · 3 comments
Open

Comments

@HoldYourWaffle
Copy link

Why does handleExtendsNodes use the Array.reduce function instead of Array.map? As far as I can see it's always a direct transformation, the purpose of Array.map.
Also, shouldn't the line m = m.concat(...) just be m.push(...)?

Using map and push instead of concat would simplify the code from this (I removed irrelevant lines):

function handleExtendsNodes (tree, options, ctx) {
  return tree.reduce((m, node) => {
    if (node.name !== 'extends') {
      if (Array.isArray(node.content)) {
        node.content = handleExtendsNodes(node.content, options, ctx)
        m.push(node)
      } else {
        m.push(node)
      }
      return m
    }

    // ... snip ...

    m = m.concat(mergeExtendsAndLayout(layoutTree, handleExtendsNodes(node.content, options, ctx), ctx))
    return m
  }, [])
}

to this:

function handleExtendsNodes (tree, options, ctx) {
  return tree.map(node => {
    if (node.name !== 'extends') {
      if (Array.isArray(node.content)) {
        node.content = handleExtendsNodes(node.content, options, ctx)
	  }
	  return node
    }

    // ... snip ...

    return mergeExtendsAndLayout(layoutTree, handleExtendsNodes(node.content, options, ctx), ctx)
  })
}

I haven't tested this code so I'm not 100% sure it works as intended, but I don't see any reason why there's the need for reduce. I might however be missing something (probably very obvious), hence the question.

@jescalan
Copy link
Member

I think you're right here, probably I was tinkering with other logic initially when I made it a reduce, then never changed it back. Wanna PR and change it, see if the tests pass?

@HoldYourWaffle
Copy link
Author

HoldYourWaffle commented Apr 19, 2019

I did some investigating, and it seems like I'm actually wrong. It's not a direct transformation because a single <block> element gets 'unwrapped' to multiple children.

I was right however about the possibility to simplify the last 2 lines:

// Current
m = m.concat(mergeExtendsAndLayout(*snip*))
return m

// Option 1
m.push(...mergeExtendsAndLayout(*snip*))
return m

// Option 2
return m.concat(mergeExtendsAndLayout(*snip*))

I think I like option 2 better, and it's probably faster too. I don't think it's worth it to create a PR for this though, I'll most likely include it in #24.

@jescalan
Copy link
Member

That sounds good to me 👍

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

No branches or pull requests

2 participants