bgrins / devtools-snippets

A collection of helpful snippets to use inside of browser devtools
http://bgrins.github.io/devtools-snippets/
2.96k stars 370 forks source link

Developer Tools Import/Export Snippet for Google Chrome #35

Closed anaran closed 10 years ago

anaran commented 11 years ago

This new functionality,also discussed with @paulirish in bgrins/devtools-snippets#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 commented 11 years ago

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 commented 11 years ago

@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.

bgrins commented 11 years ago

@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 commented 11 years ago

https://github.com/bgrins/devtools-snippets/pull/35#issuecomment-27652422

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

@bgrins https://github.com/bgrins/devtools-snippets/pull/35#issuecomment-27652590

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 commented 11 years ago

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

I haven't seen a real stopper in there.

bgrins commented 11 years ago

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.

anaran commented 11 years ago

@bgrins https://github.com/bgrins/devtools-snippets/pull/35#issuecomment-27657684 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 https://github.com/anaran/devtools-snippets/commit/60b42162301be36684a5e5ce5604e3940ac1ef41#commitcomment-4499410

Looking forward to where we can take all this.

Adrian

bgrins commented 11 years ago

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.

thomasandersen commented 11 years ago

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 https://github.com/bgrins/devtools-snippets/pull/35#issuecomment-27657684 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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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.

:+1: I think the approach @bgrins outlines above should work.

anaran commented 11 years ago

That's a lot of _could_s and _should_s against my reasoning in https://github.com/bgrins/devtools-snippets/pull/35#issuecomment-27703203

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

thomasandersen commented 11 years ago

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

anaran commented 10 years ago

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 commented 10 years ago

I just found this we should be aware of: http://code.google.com/p/fbug/issues/detail?id=6951&can=1&q=snippet&colspec=ID%20Type%20Status%20Owner%20Test%20Summary%20Reporter

thomasandersen commented 10 years ago

@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 commented 10 years ago

@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 commented 10 years ago

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

bgrins commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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

anaran commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

Thanks for testing, @thomasandersen !

See https://github.com/bgrins/devtools-snippets/pull/35#issuecomment-29243131 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 commented 10 years ago

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.

anaran commented 10 years ago

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 commented 10 years ago

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

bgrins commented 10 years ago

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 commented 10 years ago

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

anaran commented 10 years ago

@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