support multiple files for codesandbox import #5619

cyan33 · 2018-05-31T05:11:03Z

This PR adds the ability when you want to import a CodeSandbox with multiple files (like css files). So you could do something like:

[codesandbox://components-and-props/rendering-a-component.js,components-and-props/rendering-a-component.css]

However, I came across an issue when we have more than one JavaScript files and could not decide which one is the entry. Previously we just port whatever content into index.js and that becomes the entry. My current quick workaround is to allow only a single JavaScript file in the list and throw an error if it's more than one. The advantage is that it has a good backward compatibility. We don't need to change the name of the file or add extraneous logic.

Anyway, I tested with the reactjs.org repo and it works. But I'm not quite sure about the changes of the snapshot test, which causes the fail of the CI build. Do I need to just change the snapshot accordingly?

cc @bvaughn @washingtonsoares

gatsbybot · 2018-05-31T05:17:53Z

Deploy preview for gatsbygram ready!

Built with commit 4d1a232

https://deploy-preview-5619--gatsbygram.netlify.com

gatsbybot · 2018-05-31T05:19:01Z

Deploy preview for using-drupal ready!

Built with commit 4d1a232

https://deploy-preview-5619--using-drupal.netlify.com

bvaughn · 2018-05-31T20:02:19Z

However, I came across an issue when we have more than one JavaScript files and could not decide which one is the entry.

Ah. This is a good point. Here is an initial idea (that I have not thought through fully). Let's say we have the following:

[codesandbox://path/foo.js,path/bar.js,path/baz.css]

What if the package JSON we sent CodeSandbox:

Always used the same file names (bar.js, baz.js, and baz.css).
We always point the CodeSandbox at the first file passed in via the package "main" field.

So given the above example, the JSON we posted would look like:

files: {
  "package.json": {
    content: {
      dependencies: {
        react: "latest",
        "react-dom": "latest"
      },
      main: "foo.js"
    }
  },
  "foo.js": {
    content: <contents-of-foo.js>
  },
  "bar.js": {
    content: <contents-of-bar.js>
  },
  "baz.css": {
    content: <contents-of-baz.css>
  },
  "index.html": {
    content: html
  }
}

bvaughn

This needs new tests to cover the new functionality 😄

Non-CodeSandbox REPL targets should also error if given this syntax, since they don't support it. (And we should have tests for this too.)

bvaughn · 2018-06-01T17:38:25Z

packages/gatsby-remark-code-repls/src/index.js

@@ -114,17 +126,22 @@ module.exports = (
                  }
                  return map
                }, {}),
+                main: filesPaths[0].fileName,


I think we should actually use the full path, not just file name. Might be conflicts otherwise.

Non-CodeSandbox REPL targets should also error if given this syntax, since they don't support it.

My changes are all covered in if (node.url.startsWith(PROTOCOL_CODE_SANDBOX)) so I don't think it'll affect other platforms.

I think we should actually use the full path, not just file name.

I am not sure here. Do you mean we put the normalized path in the main field and the name of each files? I don't think it's gonna work that way bc codesandbox won't be able to recognize the full path in remote.

My changes are all covered in if (node.url.startsWith(PROTOCOL_CODE_SANDBOX)) so I don't think it'll affect other platforms.

Agreed that your changes don't impact other platforms, but we should warn if someone uses the unsupported syntax for other platforms.

I am not sure here. Do you mean we put the normalized path in the main field and the name of each files?

I mean the relative path people pass (e.g. path/foo.js)

Sorry I misunderstood what you meant. It makes sense now. Thanks for explanation!

bvaughn · 2018-06-01T17:38:26Z

packages/gatsby-remark-code-repls/src/index.js

            "index.html": {
              content: html,
            },
          },
        }

+        filesPaths.forEach((path, i) => {
+          const code = fs.readFileSync(path.filePath, `utf8`)
+          parameters.files[path.fileName] = {


I think we should actually use the full path, not just file name. Might be conflicts otherwise.

cyan33 · 2018-06-01T19:54:28Z

Issue with importing code example with global variables

Most code examples don't have the import stuff in the beginning of the file to keep simplicity. But it's essential to have these when porting the code to create, say a CodeSandbox. How can we achieve that in a generic way in this plugin? Here are some possible options:

Create a bootload.js file that attaches all of the global variables to window and require it in index.js
Insert related script tags in index.html and they will become global.
Create separated code examples that have import in the beginning, but it's painful to maintain two separated files which have essentially the same content. Doesn't make sense.

cc @KyleAMathews

washingtonsoares · 2018-06-03T13:03:05Z

Create a bootload.js file that attaches all of the global variables to window and require it in index.js

In the below example, how will CodeSandbox be able to properly initialize the component in the react-dom?
Perhaps the fact that react and react-dom are global variables would not be enough for some cases.

Link to example: https://reactjs.org/docs/components-and-props.html#extracting-components

cyan33 · 2018-06-04T14:58:14Z

@washingtonsoares I agree. We might do need to maintain separated examples like this?

examples/folderName/Comment-example.js
examples/folderName/Comment-example.full.js

and add ReactDOM.render in full examples.

CompuIves · 2018-06-04T15:22:15Z

Hey! Just stumbled on this issue, I like the mentioned approaches. I have some extra ideas that may be of use.

@Noviny created something called codesandboxer, you can give it a single file and the library will automatically find which files/dependencies are used by those files and create a sandbox with the used files. So it reads import/require automatically. Maybe https://github.com/Noviny/codesandboxer/tree/master/packages/codesandboxer-fs could be changed to work as an imported library as well for something like this plugin. Then you can still give it a single file, and extra files required by that file will be added automagically.

Issue with importing code example with global variables

This is a good one. I generally prefer to keep the import statements in the example code, to prevent confusion when people want to copy the code to their local editor/CodeSandbox. If you want global variables though, maybe you could change the function that generates the sandbox to give a param like this:

{
  React: 'react',
  ReactDOM: 'react-dom'
}

and it will generate code like this at the start of every file:

import React from 'react';
import ReactDOM from 'react-dom';

Just some additional ideas, I already like the proposed solution. I'm curious what you think!

hsribei · 2018-06-04T18:51:15Z

@washingtonsoares @cyan33 I couldn't find where exactly, but I've seen before an example of a runnable snippet of markdown code that, besides having line highlighting as with the highlight-range directive used here, also had line hiding.

So it would be possible to add the necessary imports, requires and ReactDOM.renders to each snippet but hide the lines where they appear when it's not the focus of the example.

washingtonsoares · 2018-06-05T23:01:58Z

@hsribei, this seems to solve the problem as a whole.

Do you have any examples of how line hiding works?

bvaughn · 2018-06-06T02:33:42Z

While working on reactjs/react.dev/pull/931, I also found myself thinking it would be nice if there was a way that we could specify a one-off dependency for a link (or override a default one- in this case, I found myself wanting to pin to React 16.3 rather than "latest").

Just food for thought. Probably not common, and definitely doesn't have to be part of this PR either.

hsribei · 2018-06-06T23:51:22Z

I tried to find examples but couldn't. I'm certain I've seen it before in some docs though. I thought it was the Rust Book but no luck :/

…

On Tue, Jun 5, 2018 at 7:01 PM Washington Soares ***@***.***> wrote: @hsribei <https://github.com/hsribei>, this seems to solve the problem as a whole. Do you have any examples about *line hiding* ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5619 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQ294ey6J1LgIXEvBUytLJs2tBM-pFks5t5w3ngaJpZM4UUbhC> .

bvaughn · 2018-07-10T17:20:31Z

packages/gatsby-remark-code-repls/src/index.js

@@ -57,25 +57,43 @@ module.exports = (

  const getFilePath = (url, protocol, directory) => {
    let filePath = url.replace(protocol, ``)
-    if (!filePath.endsWith(`.js`)) {
+    if (!filePath.endsWith(`.js`) && !filePath.endsWith(`.css`)) {


Maybe we should actually change this to be a check for files without any extension rather than this list?

bvaughn · 2018-07-10T17:22:39Z

packages/gatsby-remark-code-repls/src/index.js

+
+  const verifyFile = (path, protocol) => {
+    if (protocol !== PROTOCOL_CODE_SANDBOX && path.split(`,`).length > 1) {
+      throw Error(`Code example path should only contain a single file, but found more than one: ${path.replace(directory, ``)}`)


We might want to mention which REPLs support multi-file and which don't. This error is a little vague and could cause some confusion.

bvaughn · 2018-07-20T14:22:27Z

Friendly ping, @cyan33– You up for making those two small tweaks so we can land this?

cyan33 · 2018-07-20T16:44:41Z

@bvaughn Sorry I was occupied at another work and forgot this. I'll do it right now! 👍

KyleAMathews · 2018-07-20T17:21:05Z

Deploy preview for using-remark failed.

Built with commit 5b9b480

https://app.netlify.com/sites/using-remark/deploys/5b5218dd4ed62f2e54b70dbe

KyleAMathews · 2018-07-20T17:21:46Z

Deploy preview for using-contentful failed.

Built with commit 4d1a232

https://app.netlify.com/sites/using-contentful/deploys/5b5602cddd28ef3de8f7c8f0

KyleAMathews · 2018-07-20T17:42:34Z

Deploy preview for gatsbyjs failed.

Built with commit 50db7d8

https://app.netlify.com/sites/gatsbyjs/deploys/5b521a423813f06d7affa102

bvaughn · 2018-07-20T19:24:53Z

What's going on with CI

bvaughn · 2018-07-20T19:28:54Z

packages/gatsby-remark-code-repls/src/index.js

+      )
+    }
+
+    if (!(/\.js|css|ts|es6$/g).test(path)) {


This is a backwards breaking change. I don't think we should make it.

I think we should check for a likely file extension (e.g. indexOf(".") > 0) and if it doesn't exist, append ".js" like we used to before– and call it a day.

KyleAMathews · 2018-07-20T19:31:25Z

What's going on with CI

Oh... the usual computers breaking and stuff :-)

cyan33 · 2018-07-20T20:33:44Z

@bvaughn Updated. Thanks for the feedback.

bvaughn

Looks ok to me. Assuming CI is happy, let's merge it.

Edit If CI other than Netlify is happy, because Netlify builds seem to be failing for all of the most recent PRs that I checked.

bvaughn · 2018-07-21T16:18:23Z

Whoops! Almost forgot to update the docs (gatsbyjs.org/packages/gatsby-remark-code-repls/) to show the newly added functionality. @cyan33, would you like to do this?

cyan33 · 2018-07-21T23:30:15Z

@bvaughn My bad, forgot to update the doc. Will do.

cyan33 · 2018-07-22T22:12:41Z

packages/gatsby-remark-code-repls/README.md


 <!-- after -->
 <a href="/redirect-to-codepen/components-and-props/rendering-a-component">
  Try it on CodePen
 </a>

 <!-- before -->
-[Try it on CodeSandbox](codesandbox://components-and-props/rendering-a-component)
+[Try it on CodeSandbox](codesandbox://components-and-props/rendering-a-component.js)


Let's tell people to add explicit file extensions now that we support multiple types of files, even if we have a fallback mechanism for JavaScript files.

I think this is a good suggestion.

cyan33 · 2018-07-22T22:18:39Z

@bvaughn Sorry Brian, I accidentally override your commit changes with force push. I've run prettier and replaced indexOf with includes myself. Hope you don't mind :)

Let's see if the document looks good to you.

CompuIves

I absolutely love this! Thanks for the effort in making this support multiple files!

One question that I have (should not block merging): is it possible to define dependencies currently?

CompuIves · 2018-07-23T13:05:42Z

packages/gatsby-remark-code-repls/README.md

+CodesandBox supports code example with multiple files. With this plugin, you can do:
+
+```html
+[Try it on CodePen](codesandbox://my-example/index.js,my-example/util.js,my-example/index.css)


One nit: CodePen > CodeSandbox

CompuIves · 2018-07-23T13:06:48Z

packages/gatsby-remark-code-repls/README.md

+│   └── index.css
+```
+
+CodesandBox supports code example with multiple files. With this plugin, you can do:


Small nit: CodesandBox -> CodeSandbox

cyan33 · 2018-07-23T16:35:44Z

@CompuIves Thanks! As for your question, this PR doesn't involve any changes to dependencies. But seems like there's already a way to customize dependencies through config.

bvaughn · 2018-07-23T16:58:31Z

packages/gatsby-remark-code-repls/README.md

+
+import "./index.css"
+```
+


Nice overview.

bvaughn · 2018-07-23T16:58:49Z

packages/gatsby-remark-code-repls/src/index.js

@@ -57,25 +57,52 @@ module.exports = (

  const getFilePath = (url, protocol, directory) => {
    let filePath = url.replace(protocol, ``)
-    if (!filePath.endsWith(`.js`)) {
+    if (!filePath.includes(`.`)) {


Thanks for making this change! It was nagging me 😁

bvaughn

Looks good to me

bvaughn · 2018-07-23T17:01:38Z

I guess the ball is in your court now, @KyleAMathews / @gatsbot 😁

gatsbot · 2018-07-25T06:02:27Z

Holy buckets, @cyan33 — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. You’ll receive an email shortly asking you to confirm. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

KyleAMathews · 2018-07-25T06:04:18Z

Thanks for the nice improvement! Published in [email protected]

cyan33 force-pushed the support-multiple-files-codesandbox branch from 61a4940 to 1126275 Compare May 31, 2018 05:19

bvaughn self-assigned this May 31, 2018

support multiple files for codesandbox import

819f419

cyan33 force-pushed the support-multiple-files-codesandbox branch from 1126275 to 819f419 Compare May 31, 2018 20:46

washingtonsoares previously approved these changes Jun 1, 2018

View reviewed changes

cyan33 changed the title ~~[WIP] support multiple files for codesandbox import~~ support multiple files for codesandbox import Jun 1, 2018

bvaughn suggested changes Jun 1, 2018

View reviewed changes

add new test cases and refactor code

5596540

cyan33 dismissed washingtonsoares’s stale review via 5596540 June 1, 2018 22:00

washingtonsoares mentioned this pull request Jun 22, 2018

feat: Support hide lines in remark-embed-snippet #6084

Merged

bvaughn previously approved these changes Jul 10, 2018

View reviewed changes

cyan33 dismissed bvaughn’s stale review via 5b9b480 July 20, 2018 17:16

cyan33 force-pushed the support-multiple-files-codesandbox branch from 5b9b480 to 50db7d8 Compare July 20, 2018 17:22

bvaughn reviewed Jul 20, 2018

View reviewed changes

fix the nits

7197cd8

cyan33 force-pushed the support-multiple-files-codesandbox branch from 50db7d8 to 7197cd8 Compare July 20, 2018 20:32

bvaughn previously approved these changes Jul 21, 2018

View reviewed changes

update document about supporting multiple files

87be5f1

cyan33 dismissed bvaughn’s stale review via 87be5f1 July 22, 2018 21:54

cyan33 force-pushed the support-multiple-files-codesandbox branch from b7526cd to 87be5f1 Compare July 22, 2018 21:54

replace indexOf with includes

96b0449

cyan33 commented Jul 22, 2018

View reviewed changes

ran prettier

62702c2

cyan33 force-pushed the support-multiple-files-codesandbox branch from e1bb8b6 to 62702c2 Compare July 22, 2018 22:20

CompuIves reviewed Jul 23, 2018

View reviewed changes

CodesandBox -> CodeSandbox

4d1a232

bvaughn reviewed Jul 23, 2018

View reviewed changes

bvaughn approved these changes Jul 23, 2018

View reviewed changes

KyleAMathews merged commit e88d891 into gatsbyjs:master Jul 25, 2018

support multiple files for codesandbox import #5619

support multiple files for codesandbox import #5619

Conversation

cyan33 commented May 31, 2018 • edited Loading

gatsbybot commented May 31, 2018 • edited Loading

gatsbybot commented May 31, 2018 • edited Loading

bvaughn commented May 31, 2018

bvaughn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyan33 Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyan33 commented Jun 1, 2018

Issue with importing code example with global variables

washingtonsoares commented Jun 3, 2018 • edited Loading

cyan33 commented Jun 4, 2018 • edited Loading

CompuIves commented Jun 4, 2018 • edited Loading

hsribei commented Jun 4, 2018

washingtonsoares commented Jun 5, 2018 • edited Loading

bvaughn commented Jun 6, 2018

hsribei commented Jun 6, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jul 20, 2018

cyan33 commented Jul 20, 2018

KyleAMathews commented Jul 20, 2018

KyleAMathews commented Jul 20, 2018 • edited Loading

KyleAMathews commented Jul 20, 2018

bvaughn commented Jul 20, 2018

Choose a reason for hiding this comment

KyleAMathews commented Jul 20, 2018

cyan33 commented Jul 20, 2018

bvaughn left a comment • edited Loading

Choose a reason for hiding this comment

bvaughn commented Jul 21, 2018

cyan33 commented Jul 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyan33 commented Jul 22, 2018

CompuIves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyan33 commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn left a comment

Choose a reason for hiding this comment

bvaughn commented Jul 23, 2018

gatsbot bot commented Jul 25, 2018

KyleAMathews commented Jul 25, 2018

cyan33 commented May 31, 2018 •

edited

Loading

gatsbybot commented May 31, 2018 •

edited

Loading

gatsbybot commented May 31, 2018 •

edited

Loading

cyan33 Jun 1, 2018 •

edited

Loading

washingtonsoares commented Jun 3, 2018 •

edited

Loading

cyan33 commented Jun 4, 2018 •

edited

Loading

CompuIves commented Jun 4, 2018 •

edited

Loading

washingtonsoares commented Jun 5, 2018 •

edited

Loading

KyleAMathews commented Jul 20, 2018 •

edited

Loading

bvaughn left a comment •

edited

Loading