Address Issue174 #200

tansongyang · 2017-09-15T02:02:18Z

These commits address issue #174 . A few points:

I couldn't run eslint because it complained about line endings (I'm on Windows). We could always add a .gitattributes file to let git enforce this instead of eslint. This would make it easier for people on Windows to contribute. I can PR if you agree.
I notice that, even if there is an error, the URL still gets added in the UI. Do you want to prevent that, or leave things as they are?

Rewrite addResource to be testable and export it.

lbogdan · 2017-09-15T09:01:59Z

src/sandbox/external-resources.js

+    return getContentType(resource).then(contentType => {
+      if (contentType.indexOf('text/css') === 0) {
+        addCSS(resource);
+      } else if (contentType.indexOf('application/javascript') === 0) {


application/javascript is not the only content type for javascript source code, see here.

According to my research, application/javascript is official. We could also add application/ecmascript, since it seems like it's in use. The text/ ones are deprecated. Do you think it's OK to leave them out? The rest seem obscure.

I looked at the 'Content-Type' header for the scripts in a bunch of random websites, and the results were all over the place, from application/javascript, to text/javascript, to application/x-javascript etc.

OK. In that case, we can probably just go ahead and add the whole list.

CompuIves · 2017-09-15T14:18:43Z

Very nice! Thanks a lot 😄 . Also very nice that you wrote the tests.

I notice that, even if there is an error, the URL still gets added in the UI. Do you want to prevent that, or leave things as they are?

Hmm strange, I'd expect the error overlay to come up. I'll do some testing in a min.

CompuIves · 2017-09-15T19:41:43Z

Okay! Tested it, works great. I agree with @lbogdan that we should add the other checks to be sure.

I made a small change: now we catch the errors and show it to the user + we check if the resource exists.

Curious what you think!

tansongyang · 2017-09-15T23:09:42Z

@CompuIves I like it. I'll also add the extra checks.

tansongyang · 2017-09-16T00:04:31Z

@CompuIves @lbogdan Thanks for all the feedback! I've updated the code and tests. Is there anything else to be done?

CompuIves · 2017-09-16T13:41:15Z

Yep, one small thing left! Could you add yourself to the contributors by running:

npm run add

lbogdan · 2017-09-16T15:44:16Z

While doing some testing, I encountered a corner(?) case: what if the server that hosts the script / stylesheet doesn't have CORS enabled?

(that's why I said in the issue that it would have been much nicer for this to be handled in the API)

tansongyang · 2017-09-16T16:37:37Z

HI @lbogdan, could you please clarify what you mean by having this handled by the API? Could you also share the URL that produces the issue?

lbogdan · 2017-09-17T10:31:22Z

Let's say you have an URL without a .js (or .css) extension, but which returns javascript (or css), together with correct Content-Type header (example: javascript / css). Because that server doesn't send CORS headers, when trying to add it as a resource I get this:

It still gets added to the sandbox, but that's only because there's no error checking.

lbogdan · 2017-09-17T10:49:07Z

@tansongyang To answer your other question

could you please clarify what you mean by having this handled by the API?

When adding a resource, there's an "add resource to sandbox" API call with the resource URL, and while serving that request the API could check the content type of the resource and return it in the response (or return an error if not a valid content type, which could be very easily handled by the UI). Also, in all other API responses ("get sandbox", "get sandbox's resources"), resources should be returned as { url, type }.

The API is currently closed source, so this is something only @CompuIves can do for now, at least on the API side.

tansongyang · 2017-10-03T21:56:50Z

@lbogdan @CompuIves Just checking on the status of this PR. Do we want to scrap this and go with the API approach? Or keep it as it is and find a different solution for the case where the server doesn't have CORS enabled?

lbogdan · 2017-10-03T22:10:33Z

@tansongyang I don't really have any say in this, but if I had, I'd go with the API approach. For me it makes the most sense for the API to return the list of external resources, including their types (also, to figure out the type of a resourse, and to validate it on add). The only issue with that is that, as far as I know, @CompuIves' focus is currently on other functionality.

CompuIves · 2017-10-06T19:44:07Z

I agree that the API approach is most desirable. I'll probably work on that somewhere around December.

Until then we can use this approach and default to the old behaviour if we encounter a CORS issue.

lbogdan · 2017-10-09T10:24:21Z

Before merging, I think we should fix the issue with the resource being added anyway, even if there was an error (invalid content type or CORS). I took a quick look and saw that the resource is first added to the sandbox (through an API call), and only then loaded. We should probably do the check before adding it to the sandbox.

CompuIves · 2017-10-10T00:24:01Z

Hmm, we should add the resource regardless because we can't deny access to resources that don't have their cors headers on allow...

tansongyang · 2017-10-10T01:03:18Z

I think it's fine to just allow it as well. Isn't that what codesandbox used to do? Adding more checks seems like it's beyond the scope of this PR, which was originally intended just to allow things like Google fonts.

lbogdan · 2017-10-10T08:58:55Z

@CompuIves The check should only be for resources that don't end in .js or .css, those will work regardless. For the rest, even if we allow them to be added to the sandbox, they will error when we try to check their content type, and won't work.

lbogdan · 2018-05-01T13:11:53Z

Now that we can load scripts and style-sheets directly from index.html, should we pursue this further?

stale · 2018-12-03T12:18:03Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

SaraVieira · 2019-01-09T21:41:57Z

Closed because of lack of activity

tansongyang added 2 commits September 12, 2017 13:51

Add external-resources.test.js

a94b240

Rewrite addResource to be testable and export it.

Add Content-Type check for external resources

c401b1b

lbogdan reviewed Sep 15, 2017

View reviewed changes

Add check if resource exists

1b1d12c

Allow all JS media types

bc4aa9c

Add tansongyang to contributors

0206e3e

stale bot added the stale? label Dec 3, 2018

SaraVieira removed the stale? label Jan 8, 2019

SaraVieira closed this Jan 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address Issue174 #200

Address Issue174 #200

tansongyang commented Sep 15, 2017 •

edited

Loading

lbogdan Sep 15, 2017

tansongyang Sep 15, 2017

lbogdan Sep 15, 2017

tansongyang Sep 15, 2017

CompuIves commented Sep 15, 2017

CompuIves commented Sep 15, 2017 •

edited

Loading

tansongyang commented Sep 15, 2017 •

edited

Loading

tansongyang commented Sep 16, 2017

CompuIves commented Sep 16, 2017

lbogdan commented Sep 16, 2017 •

edited

Loading

tansongyang commented Sep 16, 2017 •

edited

Loading

lbogdan commented Sep 17, 2017 •

edited

Loading

lbogdan commented Sep 17, 2017 •

edited

Loading

tansongyang commented Oct 3, 2017 •

edited

Loading

lbogdan commented Oct 3, 2017 •

edited

Loading

CompuIves commented Oct 6, 2017

lbogdan commented Oct 9, 2017

CompuIves commented Oct 10, 2017

tansongyang commented Oct 10, 2017 •

edited

Loading

lbogdan commented Oct 10, 2017

lbogdan commented May 1, 2018 •

edited

Loading

stale bot commented Dec 3, 2018

SaraVieira commented Jan 9, 2019

Address Issue174 #200

Address Issue174 #200

Conversation

tansongyang commented Sep 15, 2017 • edited Loading

lbogdan Sep 15, 2017

Choose a reason for hiding this comment

tansongyang Sep 15, 2017

Choose a reason for hiding this comment

lbogdan Sep 15, 2017

Choose a reason for hiding this comment

tansongyang Sep 15, 2017

Choose a reason for hiding this comment

CompuIves commented Sep 15, 2017

CompuIves commented Sep 15, 2017 • edited Loading

tansongyang commented Sep 15, 2017 • edited Loading

tansongyang commented Sep 16, 2017

CompuIves commented Sep 16, 2017

lbogdan commented Sep 16, 2017 • edited Loading

tansongyang commented Sep 16, 2017 • edited Loading

lbogdan commented Sep 17, 2017 • edited Loading

lbogdan commented Sep 17, 2017 • edited Loading

tansongyang commented Oct 3, 2017 • edited Loading

lbogdan commented Oct 3, 2017 • edited Loading

CompuIves commented Oct 6, 2017

lbogdan commented Oct 9, 2017

CompuIves commented Oct 10, 2017

tansongyang commented Oct 10, 2017 • edited Loading

lbogdan commented Oct 10, 2017

lbogdan commented May 1, 2018 • edited Loading

stale bot commented Dec 3, 2018

SaraVieira commented Jan 9, 2019

tansongyang commented Sep 15, 2017 •

edited

Loading

CompuIves commented Sep 15, 2017 •

edited

Loading

tansongyang commented Sep 15, 2017 •

edited

Loading

lbogdan commented Sep 16, 2017 •

edited

Loading

tansongyang commented Sep 16, 2017 •

edited

Loading

lbogdan commented Sep 17, 2017 •

edited

Loading

lbogdan commented Sep 17, 2017 •

edited

Loading

tansongyang commented Oct 3, 2017 •

edited

Loading

lbogdan commented Oct 3, 2017 •

edited

Loading

tansongyang commented Oct 10, 2017 •

edited

Loading

lbogdan commented May 1, 2018 •

edited

Loading