Skip to content

Run only on solidus#6

Merged
localjo merged 12 commits intomasterfrom
run-only-on-solidus
Jun 27, 2014
Merged

Run only on solidus#6
localjo merged 12 commits intomasterfrom
run-only-on-solidus

Conversation

@localjo
Copy link
Copy Markdown
Member

@localjo localjo commented Jun 24, 2014

It doesn't make sense for the Solidus devtools to run on a non-Solidus page, so we need a way to check if a page is a Solidus page before the Solidus devtools runs on it. The Grunt, Facebook React, and other devtools extensions do similar checks and don't run on pages that aren't using those tools.

There are a lot of ways we can check if a page is a Solidus page. @pushred and I had discussed using the page's x-powered-by headers. Currently that header shows Express, but we could potentially change that to be more specific to Solidus. The downside of this approach would be that it requires a second XMLHttpRequest to check the headers with JavaScript. Since we have to request the page's JSON object anyway, maybe we could just check that route and make sure that it's a Solidus object before running the rest of the DevTools. That would save a request and wouldn't depend on us changing the headers.

Another option could be whether or not a page has a Solidus object, though @pushred had mentioned that the pages may not be served with a Solidus object in the future.

Perhaps a meta tag in the HTML head would be another workable option to consider.

localjo added 2 commits June 23, 2014 16:45
If not a Solidus page, InspectorJSON will not be initialized and any
existing InspectorJSON instances are destroyed.
@localjo
Copy link
Copy Markdown
Member Author

localjo commented Jun 24, 2014

I made a bunch of improvements to how messaging between pages works. Moved message processing to devpanel.js, and the messages passed from background.js are now all JSON objects to make it easier to pass complex information consistently.

With this update the plugin won't initialize InspectorJSON if the inspected window isn't a Solidus page, and if InspectorJSON was already initialized on a previously inspected page, navigating to a non Solidus page will invoke inspector.destroy(). Whether or not a page is a Solidus page is determined by whether or not there is JSON with a page key at the expected route. It might make sense to add another key to the JSON that is more specific to Solidus, since the expected JSON route and page key could conceivably be used by other platforms and cause unexpected behavior.

This update also adds a messaging panel that gives the user some information about the current status of the plugin. Styling of the message panel will be tackled with issue #2.

@localjo
Copy link
Copy Markdown
Member Author

localjo commented Jun 25, 2014

The getJsonResource() function is being called on all Chrome tab updates, not just updates to the inspected tab. That causes the InspectorJSON to get destroyed if any other non-Solidus tab updates. I need to add another check here to make sure that we only try to get the JSON for the page we're inspecting.

@pushred
Copy link
Copy Markdown
Member

pushred commented Jun 25, 2014

@joanniclaborde any thoughts here? I still think customizing the x-powered-by header (or adding an entirely new one) is going to be the safest option. See here to disable (and replace):

@joanniclaborde
Copy link
Copy Markdown

I think the x-powered-by header is a good idea, since we control them 100% (vs a js object in the page or a meta tag, which can be easily changed by the user).

@localjo
Copy link
Copy Markdown
Member Author

localjo commented Jun 26, 2014

For now, I'll update the devtools to check for X-Powered-By:Express since that should be a quick change once we make the update to the Solidus headers.

@joanniclaborde
Copy link
Copy Markdown

I'm about to commit the x-powered-by: Solidus change to Solidus, do you also need the current version? Might be useful in case of breaking changes (x-powered-by: Solidus/0.1.7).

@localjo
Copy link
Copy Markdown
Member Author

localjo commented Jun 26, 2014

It could be useful. I'm trying to decide what should happen if someone is trying to inspect an older Solidus page that's returning x-powered-by: Express. Should that just be treated as "non-Solidus", or do the devtools need to support those sites too?

@joanniclaborde
Copy link
Copy Markdown

I suggest we only support the Solidus header, if a dev wants to use the devtool on an old site, he just needs to update it first.

@localjo
Copy link
Copy Markdown
Member Author

localjo commented Jun 26, 2014

Ok. Cool. I'm now checking that the string starts with "Solidus", so we should be able to include the version number on the end. And with that I think this pull request is ready to merge. @pushred what are your thoughts?

@joanniclaborde
Copy link
Copy Markdown

See solidusjs/solidus#112

Comment thread background.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should use the strict operator just as a safe, consistent habit. Rather than nitpick style though line-by-line I'm just gonna go ahead and commit a JSHint config file to this branch that covers our standards (formal + informal). Can you set up a Grunt/Gulp task that runs it and starts the basis for tests? I can hook up CircleCI then so we can save time reviewing this sorta thing.

Comment thread background.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like to clean this sort of logic up with a descriptive condition, in this case you might consider two: isDone and isSolidus.

@pushred
Copy link
Copy Markdown
Member

pushred commented Jun 26, 2014

And with that I think this pull request is ready to merge. @pushred what are your thoughts?

Left a couple fairly trivial comments, aside from that I'd love to see the start of tests here. I know that gets a bit tricky with the browser extensions, but you should be able to mock some Solidus responses and possibly some of the Chrome APIs with Sinon.js? It'd be a lot easier to understand what all is going on vs. trying to follow the various console logs.

localjo added a commit that referenced this pull request Jun 27, 2014
@localjo localjo merged commit 4d66c7c into master Jun 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants