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.

Sign up for GitHub

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

Add an unminified production build to the NPM package #7403

Merged
merged 3 commits into from
Oct 17, 2018
Merged

Add an unminified production build to the NPM package #7403

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
58205a1
add an unminified production build to the package
mourner Oct 11, 2018
30a842c
better naming in build scripts
mourner Oct 11, 2018
ad51596
clarify build file names
mourner Oct 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
/dist/
/docs/pages/dist/mapbox-gl-dev.js
/docs/pages/dist/mapbox-gl.js
/docs/pages/dist/mapbox-gl-unminified.js
*.js.map
node_modules
package-lock.json
Expand Down
6 changes: 4 additions & 2 deletions build/rollup_plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import uglify from 'rollup-plugin-uglify';
import minifyStyleSpec from './rollup_plugin_minify_style_spec';
import { createFilter } from 'rollup-pluginutils';

const production = process.env.BUILD === 'production';
const {BUILD} = process.env;
const minified = BUILD === 'production';
const production = BUILD === 'production' || BUILD === 'production-unminified';

// Common set of plugins/transformations shared across different rollup
// builds (main mapboxgl bundle, style-spec package, benchmarks bundle)
Expand All @@ -30,7 +32,7 @@ export const plugins = () => [
// https://github.com/mapbox/mapbox-gl-js/pull/6956
ignoreGlobal: true
}),
production ? uglify() : false
minified ? uglify() : false
].filter(Boolean);

// Using this instead of rollup-plugin-flow due to
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@
"scripts": {
"build-dev": "rollup -c --environment BUILD:dev",
"watch-dev": "rollup -c --environment BUILD:dev --watch",
"build-alt": "rollup -c --environment BUILD:production-unminified",
Copy link
Member

Choose a reason for hiding this comment

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

why build-alt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just a short name for the package script (which shouldn't be called directly anyway), "alternative" build. Didn't want to name it build-production-unminified.

"build-min": "rollup -c --environment BUILD:production",
"build-flow-types": "cp build/mapbox-gl.js.flow dist/mapbox-gl.js.flow && cp build/mapbox-gl.js.flow dist/mapbox-gl-dev.js.flow",
"build-css": "postcss -o dist/mapbox-gl.css src/css/mapbox-gl.css",
Expand Down Expand Up @@ -141,7 +142,7 @@
"test-flow": "build/run-node build/generate-flow-typed-style-spec && flow .",
"test-flow-cov": "flow-coverage-report -i 'src/**/*.js' -t html",
"test-cov": "nyc --require=@mapbox/flow-remove-types/register --reporter=text-summary --reporter=lcov --cache run-s test-unit test-expressions test-query test-render",
"prepublishOnly": "run-s build-flow-types build-dev build-min build-css build-style-spec test-build",
"prepublishOnly": "run-s build-flow-types build-dev build-min build-alt build-css build-style-spec test-build",
"codegen": "build/run-node build/generate-style-code.js && build/run-node build/generate-struct-arrays.js"
},
"files": [
Expand Down
9 changes: 6 additions & 3 deletions rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@ import sourcemaps from 'rollup-plugin-sourcemaps';
import {plugins} from './build/rollup_plugins';

const version = JSON.parse(fs.readFileSync('package.json')).version;

const production = process.env.BUILD === 'production';
const outputFile = production ? 'dist/mapbox-gl.js' : 'dist/mapbox-gl-dev.js';
const {BUILD} = process.env;
const minified = BUILD === 'production';
const production = BUILD === 'production' || BUILD === 'production-unminified';
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the conflation of debug/release and minification? We could have two separate variables: BUILD controls dev vs. release, while UGLIFY (defaulting to true) controls minification

const outputFile =
minified ? 'dist/mapbox-gl.js' :
production ? 'dist/mapbox-gl-unminified.js' : 'dist/mapbox-gl-dev.js';
Copy link
Member

Choose a reason for hiding this comment

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

This still doesn't account for the matrix of BUILD/MINIFY yet :) How about

const suffix = (production ? '' : '-dev') + (minified ? '' : '-unminified');
const outputFile = 'dist/mapbox-gl' + suffix + '.js';

Copy link
Member Author

Choose a reason for hiding this comment

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

@kkaefer I wouldn't want to change names of existing builds. This suggestion would rename mapbox-gl-dev.js to mapbox-gl-dev-unminified.js.

Copy link
Member

Choose a reason for hiding this comment

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

Let's reverse the MINIFY flag and default to true, then specify MINIFY:false for the non-minified production build

Copy link
Member Author

Choose a reason for hiding this comment

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

@kkaefer and rename the current mapbox-gl.js build to mapbox-gl-min.js?

Copy link
Member

Choose a reason for hiding this comment

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

No, the suffix only gets added for unminified builds

Copy link
Member Author

Choose a reason for hiding this comment

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

@kkaefer so it would still be mapbox-gl-dev-unminified.js? I'm confused.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the confusion; I didn't realize that the dev version is unminified. What I was trying to get at is that we have 4 possible combinations of BUILD and MINIFY, but we're only producing 3 different file names; the one for BUILD:development,MINIFY:true produces the same output file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it doesn't make sense to have a minified dev build anyway, I don't think we need to worry about it. I added another commit that hopefully makes build naming logic clearer. Feel free to add any changes you feel are needed here and I'll review, and otherwise we can merge.


const config = [{
// First, use code splitting to bundle GL JS into three "chunks":
Expand Down