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

Grouped imports shouldn't have empty newline separation #405

Merged
merged 4 commits into from
Mar 6, 2019

Conversation

johno
Copy link
Member

@johno johno commented Feb 15, 2019

Before, each import is turned into its own node, even when grouped like so:

import Foo1 from "./foo"
import Foo2 from "./foo"
import Foo3 from "./foo"

So, when combining back to a string we ended up with:

import Foo1 from "./foo"

import Foo2 from "./foo"

import Foo3 from "./foo"

We now combine adjacent nodes of the same type. Though, if the node is a default export it is treated specially.

@vercel
Copy link

vercel bot commented Feb 15, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@vercel vercel bot temporarily deployed to staging February 28, 2019 18:01 Inactive
@johno johno added the 🕸️ area/tests This affects tests label Feb 28, 2019
@vercel vercel bot temporarily deployed to staging March 1, 2019 20:27 Inactive
@johno johno removed the 🕸️ area/tests This affects tests label Mar 1, 2019
@vercel vercel bot temporarily deployed to staging March 1, 2019 20:37 Inactive
@vercel vercel bot temporarily deployed to staging March 1, 2019 20:52 Inactive
@wooorm
Copy link
Member

wooorm commented Mar 1, 2019

This looks a bit unwieldy! I thought I’d try to wrap my head around what’s going on to see what I’d come up. It’s pseudo-code, so it probably won’t work, but it looks a bit cleaner!

var merged = []
var length = allNodes.length
var index = -1
var prevType
var currType
var node
var head

while (++index < length) {
  node = allNodes[index]
  currType = node.default ? 'default' : node.type

  if (currType === prevType) {
    head.value += node.value
  } else {
    merged.push(node)
    head = node
  }

  prevType = currType
}

Copy link
Member

@ChristopherBiscardi ChristopherBiscardi left a comment

Choose a reason for hiding this comment

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

lgtm. Removing the mutation made it easier for me to understand what was going on, ymmv.

diff --git a/packages/remark-mdx/extract-imports-and-exports.js b/packages/remark-mdx/extract-imports-and-exports.js
index e4ecaaf..3b006ce 100644
--- a/packages/remark-mdx/extract-imports-and-exports.js
+++ b/packages/remark-mdx/extract-imports-and-exports.js
@@ -57,34 +57,32 @@ module.exports = value => {
   // Group adjacent nodes of the same type so that they can be combined
   // into a single node later
   let currType = allNodes[0].type
-  const groupedNodes = allNodes.reduce(
-    (acc, curr) => {
-      // Default export nodes shouldn't be grouped with other exports
-      // because they're handled specially by MDX
-      if (curr.default) {
-        currType = 'default'
-        acc.push([curr])
-      } else if (curr.type === currType) {
-        acc[acc.length - 1].push(curr)
-      } else {
-        currType = curr.type
-        acc.push([curr])
-      }
-
-      return acc
-    },
-    [[]]
-  )
-
-  // Combine adjacent nodes into a single node
-  return groupedNodes
+  const groupedNodes = allNodes
+    .reduce(
+      (acc, curr) => {
+        // Default export nodes shouldn't be grouped with other exports
+        // because they're handled specially by MDX
+        if (curr.default) {
+          currType = 'default'
+          return [...acc, [curr]]
+        } else if (curr.type === currType) {
+          const lastNodes = acc.pop()
+          return [...acc, [...lastNodes, curr]]
+        } else {
+          currType = curr.type
+          return [...acc, [curr]]
+        }
+      },
+      [[]]
+    )
     .filter(a => a.length)
     .reduce((acc, curr) => {
-      const node = curr.shift()
-      curr.forEach(n => {
-        node.value += n.value
-      })
+      // Combine adjacent nodes into a single node
+      const node = curr.reduce((acc, curNode) => ({
+        ...acc,
+        value: acc.value + curNode.value
+      }))
 
-      return acc.concat([node])
+      return [...acc, node]
     }, [])
 }

packages/remark-mdx/extract-imports-and-exports.js Outdated Show resolved Hide resolved
packages/remark-mdx/test/__snapshots__/test.js.snap Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to staging March 6, 2019 17:04 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants