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

Use npm script to build and pack extension #107

Merged
merged 3 commits into from Jun 27, 2016

Conversation

stefanbuck
Copy link
Member

Based on the discussion with @josephfrazier on #101 this PR use npm script instead of gulp for building and packing the extension. Except npm run *-pack it works quite well. I'm just worried about the complexity in the package.json file. I'm undecided which version I prefer more.

@josephfrazier regarding the issue with npm run *-pack. When I run the script in the console it works, but the same script execute via npm script returns

zip error: Nothing to do! (out/chrome-octolinker-4.1.0.zip)

Any ideas?

@josephfrazier
Copy link
Member

josephfrazier commented Jun 21, 2016

Hmm, when I run npm run chrome-pack, it fails with this message:

npm ERR! Tell the author that this fails on your system:
npm ERR!     npm run chrome-dist && mkdir -p out && zip --recurse-paths out/chrome-octolinker-$npm_package_version.zip dist/ --exclude dist/ \* out/\* node_modules/\*

But when I run the suggested command above, it also fails with:

zip error: Nothing to do! (out/chrome-octolinker-.zip)

(Note that $npm_package_version is empty, because we're not running in the context of an npm script.)

It looks like the chrome-pack command was modeled after pack-repo, but since it's only including the files in dist/, there's no need to --exclude anything. In particular, dist/ is being excluded, which explains why zip says there's "Nothing to do!". Maybe chrome-pack should be this instead?

npm run chrome-dist && mkdir -p out && zip --recurse-paths out/chrome-octolinker-$npm_package_version.zip dist/

EDIT: likewise for firefox-pack, of course

@stefanbuck
Copy link
Member Author

@josephfrazier please have a look again. TA

@josephfrazier
Copy link
Member

Ah, that looks much better! Nice find on the zip -j flag to avoid having dist/ go into the zipped paths; I wasn't aware of that.

I do have one nitpick, though: My development process involves loading the unpacked Chrome extension directly from ./chrome, but with these changes, npm run chrome-build doesn't actually create ./chrome/manifest.json, so the extension fails to load. I started thinking about how to fix this, and I realized that we could do away with the dist/ directory entirely. Does that sound ok to you? Here's a commit for it: josephfrazier@b5d5300

@stefanbuck
Copy link
Member Author

My idea was to get rid of the chrome/ and firefox/ directory and use dist/ instead for development. Maybe it's just a naming issue. What about build? The only disadvantages I can think of is that pair development in Firefox and Chrome is not possible anymore, which shouldn't be a big issue. Btw npm run chrome-build produces a manifest.json file, under dist/ and not as you may expected under chrome/.

The manifest is already copied by the npm script task `chrome-manifest`
@josephfrazier
Copy link
Member

Oh, I see now. I was under the impression that the chrome and firefox directories were still used somewhere in the build process, but I now see that I was mistaken. I think it's cleaner to have a single output directory anyway, especially if we were to add support for other browsers like Safari. Merging :)

@josephfrazier josephfrazier merged commit 6d94101 into OctoLinker:master Jun 27, 2016
@stefanbuck stefanbuck deleted the master-manifest-file-II branch June 28, 2016 16:13
@stefanbuck
Copy link
Member Author

What about the naming? Should we call it build instead of dist?

@josephfrazier
Copy link
Member

I don't really have a preference there, other than a very slight preference to leave it as dist, so I don't have to change the path that Chrome loads the unpacked extension from :P

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