Skip to content

Ensure style-spec is published with mapbox-gl #6586

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

Merged
merged 3 commits into from
May 1, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions circle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ jobs:
- run: yarn run build-min
- run: yarn run build-dev
- run: yarn run build-css
- run: yarn run build-style-spec
- run: yarn run test-build
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfirebaugh do you see any reason to do test-build as a separate workflow step? I thought it kinda made sense to keep it here, but 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

No, adding it here is what I would have done too.

- persist_to_workspace:
root: .
paths:
Expand Down
8 changes: 5 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,11 @@
"sourceCache": true
},
"scripts": {
"build-dev": "rollup -c --environment BUILD:dev && build/run-tap --no-coverage test/build/dev.test.js",
"build-dev": "rollup -c --environment BUILD:dev",
"watch-dev": "rollup -c --environment BUILD:dev --watch",
"build-min": "rollup -c --environment BUILD:production && build/run-tap --no-coverage test/build/min.test.js",
"build-min": "rollup -c --environment BUILD:production",
"build-css": "postcss -o dist/mapbox-gl.css src/css/mapbox-gl.css",
"build-style-spec": "cd src/style-spec && npm run build && cd ../.. && cp -R src/style-spec/dist dist/style-spec",
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for npm rather than yarn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, just habit, will change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to npm due to mapbox/mbgl-ci-images#17

"watch-css": "postcss --watch -o dist/mapbox-gl.css src/css/mapbox-gl.css",
"build-token": "node build/generate-access-token-script.js",
"build-benchmarks": "BENCHMARK_VERSION=${BENCHMARK_VERSION:-\"$(git rev-parse --abbrev-ref HEAD) $(git rev-parse --short=7 HEAD)\"} rollup -c bench/rollup_config_benchmarks.js",
Expand All @@ -141,13 +142,14 @@
"test-suite": "run-s test-render test-query",
"test-suite-clean": "find test/integration/{render,query}-tests -mindepth 2 -type d -not \\( -exec test -e \"{}/style.json\" \\; \\) -print | xargs -t rm -r",
"test-unit": "build/run-tap --reporter classic --no-coverage test/unit",
"test-build": "build/run-tap --no-coverage test/build/**/*.test.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this get run in CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, good catch -- accidentally omitted the circle.yml change

"test-render": "node --max-old-space-size=2048 test/render.test.js",
"test-query": "node test/query.test.js",
"test-expressions": "build/run-node test/expression.test.js",
"test-flow": "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",
"prepublish": "in-publish && run-s build-dev build-min build-css || not-in-publish",
"prepublish": "in-publish && run-s build-dev build-min build-css build-style-spec test-build || not-in-publish",
"codegen": "build/run-node build/generate-style-code.js && build/run-node build/generate-struct-arrays.js"
},
"files": [
Expand Down
77 changes: 32 additions & 45 deletions test/build/style-spec.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
const test = require('mapbox-gl-js-test').test;
const fs = require('fs');
const path = require('path');
const exec = require('child_process').exec;
const isBuiltin = require('is-builtin-module');

const Linter = require('eslint').Linter;
Expand All @@ -15,55 +14,43 @@ import rollupConfig from '../../src/style-spec/rollup.config';
// some paths
const styleSpecDirectory = path.join(__dirname, '../../src/style-spec');
const styleSpecPackage = require('../../src/style-spec/package.json');
const styleSpecDistBundle = path.join(styleSpecDirectory, styleSpecPackage.main);
const styleSpecDistBundle = fs.readFileSync(path.join(__dirname, '../../dist/style-spec/index.js'), 'utf-8');

test('@mapbox/mapbox-gl-style-spec npm package', (t) => {
// simulate npm install of @mapbox/mapbox-gl-style-spec
t.test('build plain ES5 bundle in prepublish', (t) => {
t.tearDown(() => {
fs.unlink(styleSpecDistBundle, (error) => {
if (error) console.error(error);
});
});

exec(`rm -f ${styleSpecDistBundle} && npm run build`, { cwd: styleSpecDirectory }, (error) => {
t.error(error);
t.ok(fs.existsSync(styleSpecDistBundle), 'dist bundle exists');
const linter = new Linter();
const messages = linter.verify(styleSpecDistBundle, {
parserOptions: {
ecmaVersion: 5
},
rules: {},
env: {
node: true
}
}).map(message => `${message.line}:${message.column}: ${message.message}`);
t.deepEqual(messages, [], 'distributed bundle is plain ES5 code');

t.stub(console, 'warn');
rollup.rollup({
input: `${styleSpecDirectory}/style-spec.js`,
plugins: [{
resolveId: (id, importer) => {
if (
/^[\/\.]/.test(id) ||
isBuiltin(id) ||
/node_modules/.test(importer)
) {
return null;
}

const linter = new Linter();
const messages = linter.verify(fs.readFileSync(styleSpecDistBundle, 'utf-8'), {
parserOptions: {
ecmaVersion: 5
},
rules: {},
env: {
node: true
t.ok(styleSpecPackage.dependencies[id], `External dependency ${id} (imported from ${importer}) declared in style-spec's package.json`);
return false;
}
}).map(message => `${message.line}:${message.column}: ${message.message}`);
t.deepEqual(messages, [], 'distributed bundle is plain ES5 code');

t.stub(console, 'warn');
rollup.rollup({
input: `${styleSpecDirectory}/style-spec.js`,
plugins: [{
resolveId: (id, importer) => {
if (
/^[\/\.]/.test(id) ||
isBuiltin(id) ||
/node_modules/.test(importer)
) {
return null;
}

t.ok(styleSpecPackage.dependencies[id], `External dependency ${id} (imported from ${importer}) declared in style-spec's package.json`);
return false;
}
}].concat(rollupConfig[0].plugins)
}).then(() => {
t.end();
}).catch(e => {
t.error(e);
});
}].concat(rollupConfig[0].plugins)
}).then(() => {
t.end();
}).catch(e => {
t.error(e);
});
});

Expand Down