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

Master manifest file #101

Closed
wants to merge 4 commits into from

Conversation

stefanbuck
Copy link
Member

@stefanbuck stefanbuck commented Jun 18, 2016

Work in progress and branched off from #100

Based on the comment #100 (comment) this PR adds gulp which allows us to extend the manifest file with additional information.

New commits on top of #100 are

@stefanbuck stefanbuck force-pushed the master-manifest-file branch 11 times, most recently from ab33345 to 3dfa20f Compare June 19, 2016 12:32
@josephfrazier
Copy link
Member

Wow, I wasn't expecting so much effort to go into generating the manifest files, but it looks like there's some other changes here too. I'm wondering if we really need gulp and all the reorganization, though. For example, we could generate chrome/manifest.json from firefox/manifest.json with this npm script (using the json package):

"build-chrome-manifest": "json -e 'delete this.applications' < firefox/manifest.json > chrome/manifest.json",

Likewise, the pack-repo task could be simplified to this npm script:

"pack-repo": "mkdir -p out && zip --recurse-paths out/octolinker-repository-$npm_package_version.zip * --exclude dist/\\* out/\\* node_modules/\\*",

What do you think?

@stefanbuck
Copy link
Member Author

Mhhh to be honest, I'm a bit prejudiced, as I worked a couple of hours to get it working 😄 I see you point and in general I totally agree, but maybe there is some good reason to use gulp – let me think ...

@stefanbuck
Copy link
Member Author

The firefox mainfest.json needs an additional permissions property https://github.com and chrome don't. Both need the permission https://githublinker.herokuapp.com/. How can we achieve that with https://www.npmjs.com/package/json?

@josephfrazier
Copy link
Member

Oh, I definitely understand not wanting to let your work go to waste 😄 I certainly have no problem with using gulp, but it just struck me as overkill.

Nice catch on the permissions difference between firefox/chrome manifest.json. I wasn't sure if that was intentional or not. Anyway, we could remove it from the chrome manifest.json by changing the npm script to this:

"build-chrome-manifest": "json -e 'delete this.applications; this.permissions.shift()' < firefox/manifest.json > chrome/manifest.json",

To tie in what I was saying about the package.json version in #103 (comment), here's a script that copies the version property from package.json to firefox/manifest.json:

"update-firefox-manifest": "json -I -f firefox/manifest.json -e \"this.version = '`json version < package.json`'\"",

@stefanbuck
Copy link
Member Author

I'm gonna close this in favor of #107

@stefanbuck stefanbuck closed this Jun 27, 2016
@stefanbuck stefanbuck deleted the master-manifest-file branch June 27, 2016 22:18
josephfrazier added a commit to josephfrazier/browser-extension that referenced this pull request Aug 10, 2016
Resolves OctoLinker#156

It turns out that the only thing preventing this from working (other
than permissions) is determining the path associated with the Blob.
Before, the path was undefined, so the Blob was not initialized
properly. With this change, the path will be set to the filename entered
into the gist.

Assuming that that the filename is set to something reasonable, either
the filename-based language detection or the github-based language
detection will work correctly (at least, for non-relative links).

Here's a test page:
https://gist.github.com/josephfrazier/113827963013e98c6196db51cd889c39

---

Note that we may want to change the manifest.json `permissions` handling
at https://github.com/OctoLinker/browser-extension/blob/ec499e065424b5204885624a297ab463c99fa8ab/package.json#L17

See here for prior discussion: OctoLinker#101 (comment)
@josephfrazier josephfrazier mentioned this pull request Aug 10, 2016
stefanbuck pushed a commit that referenced this pull request Aug 11, 2016
* Add gist support

Resolves #156

It turns out that the only thing preventing this from working (other
than permissions) is determining the path associated with the Blob.
Before, the path was undefined, so the Blob was not initialized
properly. With this change, the path will be set to the filename entered
into the gist.

Assuming that that the filename is set to something reasonable, either
the filename-based language detection or the github-based language
detection will work correctly (at least, for non-relative links).

Here's a test page:
https://gist.github.com/josephfrazier/113827963013e98c6196db51cd889c39

---

Note that we may want to change the manifest.json `permissions` handling
at https://github.com/OctoLinker/browser-extension/blob/ec499e065424b5204885624a297ab463c99fa8ab/package.json#L17

See here for prior discussion: #101 (comment)

* blob-reader: add test for gist blob path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants