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

feat: add lib and es6 bundles, use closure compiler, remove gulp #70

Merged
merged 21 commits into from
Jul 16, 2018

Conversation

frederickfogerty
Copy link
Contributor

@frederickfogerty frederickfogerty commented Jul 10, 2018

Description

This PR improves the build step of this library. The minified JS build is reduced from 18K -> 11K = 40% smaller. The css size is unchanged.

There have been some changes to the source code to support the closure compiler. There might be a chance that things that worked before may not work now due to the optimisations it performs. I have tried to cover all common usage, but there may be some edge cases which I have not tested. All the tests still pass, and I've ensured that the tests test the compiled code.

Gulp has also been removed in favour of npm scripts.

New Feature

  • If this is a big feature with breaking changes, consider [opening an issue][issues] to discuss first. This is completely up to you, but please keep in mind that your PR might not be accepted.
  • Run unit tests to ensure all existing tests are still passing
  • Add new passing unit tests to cover the code introduced by your PR
  • Update the readme
  • Add some steps so we can test your cool new feature!
  • The PR title is in the conventional commit format: e.g. feat(<area>): added new way to do this cool thing #issue-number
  • Add your info to the contributors array in package.json!

Steps to Test

  1. Clone this PR and run npm install to update dependencies
  2. Run npm run build and open index.html in a browser, and test that Drift still works normally
  3. Create a consuming package, npm link, and ensure that Drift works when importing using import Drift from 'drift-zoom'

Copy link
Contributor

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

A few small changes, but by and large this looks good to me. I don't have a ton of experience with these particular build/rollup systems, though, so I'd love to get a second opinion in here first if we can.

const path = require("path");
const env = require("yargs").argv.env; // use --env with webpack 2
const pkg = require("./package.json");
const UglifyJsPlugin = require("uglifyjs-webpack-plugin");
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't appear to be used in this file.

bower.json Outdated
@@ -25,7 +25,6 @@
"src",
"test",
"CONTRIBUTING.md",
"gulpfile.js",
"index.html",
"karmaConfig.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the files in this list need updating

CONTRIBUTING.md Outdated
gulp test-local # run the test against local browsers only (Chrome, Safari, Firefox)
gulp test-full # run the test suite against all supported browsers (using SauceLabs)
npm run build # build Drift from source
npm run build:watch # watch for changes and build automatically
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Handsome tab spacing is broken here

@fleck
Copy link
Contributor

fleck commented Jul 15, 2018

👏This looks awesome! Also, that run-p package seems really nice. Looks as though it would make working with scripts much easier.

@frederickfogerty frederickfogerty merged commit e48daa7 into master Jul 16, 2018
@frederickfogerty frederickfogerty deleted the fred/use-webpack branch July 16, 2018 21:17
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.

4 participants