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

Developer Tools Import/Export Snippet for Google Chrome #35

Closed
wants to merge 0 commits into from

Conversation

anaran
Copy link
Contributor

@anaran anaran commented Nov 2, 2013

This new functionality,also discussed with @paulirish in #28, is ready to pull from my end.

I tested it in chrome stable and chrome canary (both on Windows XP).

See
https://github.com/anaran/devtools-snippets/blob/master/snippets/devtools_import_export/README.md#step-1
for a poor man's slide show.

Regards,
Adrian

@addyosmani
Copy link

Nice work on the instructions - wasn't expecting them to be so comprehensive! @bgrins might suggest squashing your commits to make it easier to review.

I'll see if I can put together an upstream crbug issue to help us work out a way export/import can be simplified on the DevTools side.

@anaran
Copy link
Contributor Author

anaran commented Nov 3, 2013

@addyosmani
Thanks for the feedback, Addy!

I am now in the process of relocating my recent work to a branch and will offer it as a squashed commit in anaran/master to make @bgrins's and any other reviewer's life easier.


[end](#step-13) [forward](#step-2) [overview](#typical-use-cases)

Open Source Tab with `Ctrl+Shift+I` and `Ctrl+3`
Copy link
Owner

Choose a reason for hiding this comment

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

Should be ctrl+shift+i / cmd+alt+i (for Mac support as well). Also, for some reason neither ctrl+3 or cmd+3 seem to work for me (on canary at least) - you may just say "Open the 'Sources' tab instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I should not be replicating keyboard shortcut help. I'll probably just refer to it and use the symbolic description as you suggested.

@bgrins
Copy link
Owner

bgrins commented Nov 3, 2013

@anaran, nice job figuring out and documenting this process :). I've gone through the process myself and left a couple notes for you guys at places I got a little stuck.

@anaran
Copy link
Contributor Author

anaran commented Nov 3, 2013

#35 (comment)

Thanks for actually going through the instructions, Brian!

I will respond individually.

Is there a short way for responding to a specific comments in github?

I am currently pasting the [commented] hyperlink in full.

@bgrins
Copy link
Owner

bgrins commented Nov 3, 2013

Also, looking at this exported file, it looks like it is pulling all of the localStorage entries from devtools. Maybe we could consider importing / exporting only the scriptSnippets object to make it both a more straightforward export format, and also something that could potentially be more portable between different developer tools.

@bgrins
Copy link
Owner

bgrins commented Nov 3, 2013

As for replying to individual messages, I'm not completely sure - but I think there should be a button to "Add a line note" below my inline comments.

@anaran
Copy link
Contributor Author

anaran commented Nov 3, 2013

@bgrins #35 (comment)

could potentially be more portable between different developer tools

Are you thinking of different browsers like Firefox here?

I think having a full backup of localStorage is useful.
It also contains consoleHistory, revision-history, etc.

@anaran
Copy link
Contributor Author

anaran commented Nov 3, 2013

@bgrins
How much of these issues need to be resolved before you can pull?

I haven't seen a real stopper in there.

  • Update README.md to reference canonical keyboard help from Google sites.
  • Define potentially portable functionality between different developer tools
    • Which functionality?
    • Which developer tools?

@bgrins bgrins mentioned this pull request Nov 3, 2013
@bgrins
Copy link
Owner

bgrins commented Nov 3, 2013

Are you thinking of different browsers like Firefox here?

Portability is a great goal. @thomasandersen has been making some progress with a Firebug extension to import and export snippets (see Issue #32), and I would love to be able to bulk import snippets into Firefox DevTools. That said, I think a JSON object with a list of snippets containing names and content is pretty much all we would need, so I don't see defining a format as a stopper. I'm only suggesting that when we export, there is a little more abstraction between what Chrome is doing right now with localStorage, and how we store them. So, an example json file may look like this:

{
  "snippets": [
    {
      "name": "snippet 1",
      "content": "document.body.whatever"
    },
    {
      "name": "snippet 2",
      "content": "alert('hi')"
    }
  ]
}

There are only two differences to what Chrome is already exporting here - changes 'snippets' to 'scriptSnippets', and removes the 'id' field. In my opinion moving some very small bits of logic into the importer and exporter would be make the format more resilient to changes that Chrome may make in the future with how they implement the snippet storage, in addition to being more portable.

When you run devtools_import_export.js from devtools docked to a page you can see how many localStorage entries it has and export them. Quite handy for developing with localStorage.

Regarding getting a full backup of local storage, I see this as a separate snippet. I think that could make this snippet export/import process easier to do (it removes a decision from the process).

It also contains consoleHistory, revision-history, etc.

I have some concern that this could have side effects on the DevTools - for instance, saved breakpoints may not make sense from one browser to another, other values may not be intended to be set manually, etc. In addition to this, I am hoping I can add a build step to this project that generates a devtools-snippets.json file to import all of them - and this export would not contain any of these additional fields.

Again, thanks for taking this on @anaran. I'm happy to hear other opinions and discuss further.

@@ -52,6 +52,7 @@ try {
var i18nForInnerText = function(select, messages) {
var nds = document.querySelectorAll(select);
for (i = 0, len = nds.length; i < len; i++) {
// .innerText contains newline characters in Chrome, while .textContent loses them.
Copy link
Owner

Choose a reason for hiding this comment

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

Is this comment saying that we should be changing the code to textContent? Seems like innerText is actually what we want here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just documented why I made the change to use innerText.

These two comment lines are actually related to snippets/html_i18n_content/html_i18n_content.js.

I hope they can stay part of this pull request even though they are not related to devtools_import_export.

Copy link
Owner

Choose a reason for hiding this comment

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

OK, that's fine - just making sure they weren't left in on accident.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think it dawns on me now:

I have formulated the comment in such a neutral way that it is not clear which of the two is desired.

@anaran
Copy link
Contributor Author

anaran commented Nov 4, 2013

@bgrins #35 (comment)
Let's see what @thomasandersen has to say on our discussions of today.

I wouldn't want to invent yet another export format until we know what specific problems we are solving.

Even if the snippet storage format in localStorage would diverge between Chrome version we would have to analyze and adapt so we are able to import snippets from either format.

An intermediate export format would still have to be adapted to that change.

I can see that at some point it would be of benefit to have a single interface, but let's wait until that point is reached.

Right now I have to special-case use of createFile and deleteFile because the code in inspect.js changed between Chrome 30 and 32. The export format stayed the same, yet I have to cater to different Chrome versions.

See
anaran@60b4216#commitcomment-4499410

Looking forward to where we can take all this.

Adrian

@bgrins
Copy link
Owner

bgrins commented Nov 4, 2013

I wouldn't want to invent yet another export format until we know what specific problems we are solving.

We need to define some format as part of this PR :). The format you have it exporting to right now is only trivially different than what I proposed. We can stick with the scriptSnippets name if you prefer. And ideally the importer could live without the id attribute, but we could follow up later to make that change.

The most thing important thing to me is that we can generate a file as part of a build step in this project that will be compatible with this importer. If there is some extra benefit to including additional things in the export from this snippet, that is fine. I will work on coming up with an exported file with all of our snippets in this project (without the extra fields) and see if it loads as expected.

Of secondary importance, I think the the overall workflow would be easier if we separated the full localStorage export / import utility into a separate snippet and limited this to only running against devtools window. If everyone would rather land this in the current form and make UI changes over time, that is fine with me.

}
};

function createSnippet(fileName, result, review) {
Copy link
Owner

Choose a reason for hiding this comment

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

Just curious, did you try using the method as @paulirish outlined in this comment: #28 (comment) - just setting the localStorage scriptSnippets item? Though it is impressive that you reverse engineered the whole devtools snippet storage API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am actually only calling into the internal devtools API via p = WebInspector.scriptSnippetModel.project();

I have not used @paulirish's suggestion to just setItem('scriptSnippets') because I never wanted to lose any existing snippets in devtools.

@thomasandersen
Copy link

I agree, a great goal should be portability among devtools. A user should be able to export from Firefox DevTools and import to eg. IE DevTools and vica versa.

If Chrome DevTools already has a format and other native tools have not I see no problem basing the devtools-snippets format from that, but for a starting point I think a devtools-snippets Snippet should only be require name:String and content:String as described in #35 (comment)
Extensions can be done later or by the exporter/importer.

snippets or scriptSnippets is the same for me, but I would go for snippets as it is simpler.
This project is hopefully only about snippets ;)

For my own extension I will adapt to the devtools-snippets model

@anaran
Copy link
Contributor Author

anaran commented Nov 4, 2013

snippets or scriptSnippets is the same for me, but I would go for snippets as it is simpler.
This project is hopefully only about snippets ;)

scriptSnippets is definitely more descriptive and the de-facto standard defined by multiple Google Chrome versions 30, 32).

@thomasandersen Are there any such de-facto standards in firefox or IE?

I think we should base this work on facts.

So far snippets is only simpler.

That's too little to go by for me.

@thomasandersen
Copy link

scriptSnippets is definitely more descriptive and the de-facto standard defined by multiple Google Chrome versions

Ok. snippets is good in context of this project, but if Chrome DevTools has already named it scriptSnippets then scriptSnippets it is. No problem.

Are there any such de-facto standards in firefox or IE?

Not that I know of.

I think we should base this work on facts.

I agree.

So far snippets is only simpler.

See my first answer.

@paulirish
Copy link

Just curious, did you try using the method as @paulirish outlined
I have not used @paulirish's suggestion to just setItem('scriptSnippets') because I never wanted to lose any existing snippets in devtools.

You could just merge the two objects, then?

http://stackoverflow.com/questions/171251/how-can-i-merge-properties-of-two-javascript-objects-dynamically

@anaran
Copy link
Contributor Author

anaran commented Nov 4, 2013

Hi @paulirish
Have you looked at my approach?

When I take over management of localStorage.scriptSnippets then I

  1. take over responsibility of maintaining id property consistency
  2. bypass existing Google Chrome devtools APIs which can already createFile, deleteFile, rename.

@bgrins
Copy link
Owner

bgrins commented Nov 4, 2013

When I take over management of localStorage.scriptSnippets then I
take over responsibility of maintaining id property consistency
bypass existing Google Chrome devtools APIs which can already createFile, deleteFile, rename.

This is true, but the logic for maintaining proper ID consistency should be simpler than accessing internal APIs used by devtools. These internal functions are also more likely to break as new versions come out.

I think it would work to have some logic that checked each new snippet for an existing snippet by name and if there is, simply replace the content field with the new contents, otherwise append this object onto the list with an id of biggestID + 1 or similar. This logic could be expanded on to add new functionality (or even simplified - you could drop the matching altogether and just add a new snippet to the store for every new snippet passed in).

@addyosmani
Copy link

This is true, but the logic for maintaining proper ID consistency should be simpler than accessing internal APIs used by devtools. These internal functions are also more likely to break as new versions come out.

👍 I think the approach @bgrins outlines above should work.

@anaran
Copy link
Contributor Author

anaran commented Nov 5, 2013

That's a lot of _could_s and _should_s against my reasoning in #35 (comment)

Do you disagree with that reasoning?

Using an internal API in inspector.js seems still a lot better to me than second-guessing what it is and will be doing with and to a likewise internal data format stored in localStorage.scriptSnippets.

@bgrins
Copy link
Owner

bgrins commented Nov 5, 2013

Using an internal API in inspector.js seems still a lot better to me than second-guessing what it is and will be doing with and to a likewise internal data format stored in localStorage.scriptSnippets.

The data format is much less likely to change, and if it does, there will surely be code in devtools to handle the migration from the old format to new (since everyone has their snippets in the old format already).

@paulirish
Copy link

Agreed. If devtools changes this localStorage key, we'll have to migrate
everyone's content. It won't be easy, so there is less likelihood we'd do
that.

Meanwhile inspector.js can change very easily (and does).

bgrins added a commit that referenced this pull request Nov 7, 2013
@bgrins
Copy link
Owner

bgrins commented Nov 7, 2013

OK, took a shot at generating the snippets.json file: https://github.com/bgrins/devtools-snippets/blob/master/snippets.json.

@thomasandersen
Copy link

For devtools/snippets in general I think it looks good.
*snippets

console.timeEnd('read of ' + file.name);
var result = readEvent.target.result;
try {
var scriptSnippets = JSON.parse(JSON.parse(result).scriptSnippets);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

scriptSnippets is stringified, so I need to parse once again.

@anaran
Copy link
Contributor Author

anaran commented Nov 24, 2013

Hi Brian, this looks pretty good already.

For your format to be compatible with a localStorage.scriptSnippets entry you would have to stringify the value once more.

I think it would be confusing if we would support both forms, scriptSnippets as a string and as an object.

See https://github.com/bgrins/devtools-snippets/pull/35/files#r7877589.

@anaran
Copy link
Contributor Author

anaran commented Nov 24, 2013

@thomasandersen
Copy link

@anaran I created that thread ;-) I have not been able to follow up on my own little project because of the lack of time (as usual).
Anyway, the Firebug team have shown interest in looking into having a native support for code snippets. It will probably be discussed more after version 1.13

@anaran
Copy link
Contributor Author

anaran commented Nov 24, 2013

@thomasandersen I know, was a bit surprised you didn't mention it here.

@bgrins I'm working on a major simplification, just supporting snippets, as was suggested.

@addyosmani Filed a Chrome canary crash bug along the way: https://code.google.com/p/chromium/issues/detail?id=323031

All: What should we do about the fact that chrome makes it pretty inconvenient to download files with .js extension (malware warning with two confirmations per file)?

How about saving snippets with .txt extension?

@anaran
Copy link
Contributor Author

anaran commented Nov 24, 2013

Individual snippets that is, .json should be fine for a scriptSnippets export.

@bgrins
Copy link
Owner

bgrins commented Nov 25, 2013

All: What should we do about the fact that chrome makes it pretty inconvenient to download files with .js extension (malware warning with two confirmations per file)?

How about saving snippets with .txt extension?

Importing an individual snippet would not work by downloading the .js file anyway (it would need to live in the JSON format for the import to work). Most likely for one snippet you would just copy/paste the content into a new snippet using the frontend. As far as bundling individual snippets or custom groups for import, we could build a little tool on the site that lets you select certain snippets and builds the JSON / gives a download link.

@anaran
Copy link
Contributor Author

anaran commented Nov 25, 2013

@bgrins My import has always supported plain source file import (multiple, also with drag/drop support) or a full or partial export of localStorage.

It cannot currently handle your .json file which does not stringify scriptSnippets.

Please see how you like this commit I just did.

Documentation will have to follow (and get a lot simpler).

Sorry for the messy commits, I may be able to clean this up a bit.

@thomasandersen
Copy link

@anaran as you probably see the issue is mainly about the tool/app, not the import/export format. Since it is so early and there has not been any response from the FB team yet, I decided to wait with mentioning it here. The intention is to not spawn more threads than necessary in your pull request ;)
The core FB team has expressed interest in having native snippet support in Firebug, discussion starting from here.
If it is of interest I suggest keeping an eye on the thread or even better join the discussion.

@anaran
Copy link
Contributor Author

anaran commented Nov 25, 2013

Thanks for the update @thomasandersen !

So this is what my latest commit looks like:

image

I tricked a bit to get the devtools-snippets in there.

@addyosmani
Copy link

Just wanted to say the above implementation looks fantastic and very usable from a UX standpoint. Nice work!

@anaran
Copy link
Contributor Author

anaran commented Nov 26, 2013

Thanks @addyosmani !

@bgrins The discussion is very good, but my pull request has become a huge mess.

I will update documentation and then open a new tidy pull request based on my devtools_import_export branch.

@thomasandersen
Copy link

The UI looks very nice. Labels like "What if snippets by those names exist in devtools?" is guide full and easy to understand

I ran into one import issue though,
When trying to import this https://github.com/bgrins/devtools-snippets/blob/master/snippets.json as local file
the content of that file was imported into one file/snippet
I expected one snippet file pr. entry in snippets.json

Tested on OS X 10.9, Chrome 31.0 and the current code

@anaran
Copy link
Contributor Author

anaran commented Nov 26, 2013

Thanks for testing, @thomasandersen !

See
#35 (comment)
for the issue you noticed.

I can see that the format @bgrins chose is more appealing to humans, but the key should not be scriptSnippets because that already has a specific meaning inside devtool's localStorage.

How about calling the key scriptSnippetsParsed or something similar?

If we agree then I can update devtools_import_export to support that as well.

@bgrins
Copy link
Owner

bgrins commented Nov 26, 2013

How about calling the key scriptSnippetsParsed or something similar?

I can rename it to just snippets, if you would rather import it this way. I don't think anyone is using the file at the moment, so I can go ahead and make this change.

bgrins added a commit that referenced this pull request Nov 26, 2013
@anaran
Copy link
Contributor Author

anaran commented Nov 26, 2013

OK then (although I prefer more specific names) lets settle on key snippets in the format you have demonstrated, which is the format of localStorage.scriptSnippets parsed, and the id properties removed.

I am still working on the documentation update.

@anaran
Copy link
Contributor Author

anaran commented Nov 26, 2013

Ah, would you already have a canonical URL for the official sippets.json file (for my documentation)?

@bgrins
Copy link
Owner

bgrins commented Nov 26, 2013

Ah, would you already have a canonical URL for the official sippets.json file (for my documentation)?

Yes, the URL is here: http://bgrins.github.io/devtools-snippets/snippets.json

@anaran
Copy link
Contributor Author

anaran commented Nov 26, 2013

Thanks for the URL. Perhaps we should offer a https URL in the future?

@anaran
Copy link
Contributor Author

anaran commented Nov 28, 2013

@bgrins
Superseded by #44
which should be good to go now.

Thanks for all the feedback @addyosmani @bgrins @thomasandersen @paulirish

Here is what I came up with in order to leave my feature branches (e.g. devtools_import_export) historically correct:

git checkout master
git status # all clean
git checkout -B issue35squashed master
git merge --squash devtools_import_export
git status # looks good
git commit # review and commit via emacs
git log --graph --abbrev-commit --stat --pretty --decorate=full --branches
git push --all -v

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

Successfully merging this pull request may close these issues.

5 participants