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

build(package): refactoring build scripts with project ref / incremental #5032

Merged
merged 9 commits into from
Oct 15, 2019

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Sep 23, 2019

Description:
This PR aims to resolve to improve our build scripts, for perf & simplicity both. It have number of changes and aim to satisfy compat as much (non-major-breaking). Still I may able to miss some, requires eye to review changes.

Changes are below:

  • Use project references / incremental
    npm run compile is now default command for daily dev workflow. It uses tsc's incremental and project references, will try to compile minimal changes as much as it can detect.

  • Simplifying npm scripts for build / package
    npm run compile : just run compiler to check build passes
    npm run build:package: run postprocess (rollup minify, subpath aliases..) to prepare actual package publish. Don't need to care about complex build steps anymore.

  • Using top-level package publishing instead of manual build steps
    all build outputs are placed under dist/* and now it is expected to publish right away from top-level root of repo.

  • Subpath alias now generated by scripts (bulid:package)
    Previously we articulated structures to lay out top-level of package to be cjs output, and then copy whole src, then manually modify sourcemap url to conform those structures - and cjs output included subpath alias package.json. Now we publish top-level right away, simply generate those alias on-the-fly when package script runs.

  • dist/esm2015 -> dist/esm
    It's preparation of real-esm-module publishing, as it is no longer 2015. it should affect any runtime as we have module / esm2015 points correct entrypoint per each unless someone explicitly do subpath import which is unsupported sceanario.

  • use files in package.json
    It's hard to block lot of files in .npmignore, instead we use package.json's files property to allow pick up what we want to include. This allows we can use dist to emit whatever output we want without clearing before publish as it won't be picked up ever.

  • emits declarationMap
    now editors can navigate to source of types instead of type definition when try to locate definitions.

Since we're doing alpha for 7.0, I think it's good chance we can try these changes to collect feedback if we see any breaking changes need to be fixed before beta / public release.

@kwonoj kwonoj requested review from benlesh and cartant September 23, 2019 05:44
@@ -1,8 +1,11 @@
{
"name": "@reactivex/rxjs",
"name": "rxjs",
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 this PR expects publish from top level root, pkg name property changed.

"test:browser": "echo \"Browser test is not working currently\" && exit -1 && npm-run-all build:spec:browser && opn spec/support/mocha-browser-runner.html",
"test:circular": "dependency-cruise --validate .dependency-cruiser.json -x \"^node_modules\" dist/esm5",
"test:systemjs": "node integration/systemjs/systemjs-compatibility-spec.js",
"test:side-effects": "check-side-effects --test integration/side-effects/side-effects.json",
"test:side-effects:update": "npm run test:side-effects -- --update",
"tests2png": "mkdirp docs_app/content/img && mocha --opts spec/support/tests2png.opts \"spec/**/*-spec.ts\""
"tests2png": "mkdirp docs_app/content/img && mocha --opts spec/support/tests2png.opts \"spec/**/*-spec.ts\"",
"compile": "tsc -b --verbose ./src/tsconfig.cjs.json ./src/tsconfig.esm.json ./src/tsconfig.esm5.json ./src/tsconfig.esm5.rollup.json ./src/tsconfig.types.json ./spec/tsconfig.json",
Copy link
Member Author

Choose a reason for hiding this comment

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

single compile runs over all config with incremental build.

"npm": ">=2.0.0"
},
"typings": "./dist/package/Rx.d.ts"
"files": [
Copy link
Member Author

Choose a reason for hiding this comment

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

this property will allow specific files to be included in published package.

@@ -1,8 +1,8 @@
--require ts-node/register
Copy link
Member Author

Choose a reason for hiding this comment

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

we still need ts-node for path mapping resolve with tsconfig-paths.

}
},
"exclude": [
"./internal/umd.ts"
Copy link
Member Author

Choose a reason for hiding this comment

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

cjs doesn't emit umd entrypoint anyway, but this makes compiler confuses to retry incremental build always (cause incremental build checks build output exists). simply exclude it so incremental build works properly.

"test": "cross-env TS_NODE_PROJECT=spec/tsconfig.json mocha --opts spec/support/default.opts \"spec/**/*-spec.ts\"",
"test_no_cache": "cross-env TS_NODE_PROJECT=spec/tsconfig.json TS_NODE_CACHE=false mocha --opts spec/support/default.opts \"spec/**/*-spec.ts\"",
"test_transpile_only": "cross-env TS_NODE_PROJECT=spec/tsconfig.json TS_NODE_TRANSPILE_ONLY=true mocha --opts spec/support/default.opts \"spec/**/*-spec.ts\"",
"test": "npm run compile && shx cp -Rf ./dist/cjs ./dist/src && mocha --opts spec/support/default.opts \"dist/spec/**/*-spec.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.

spec have imports to src (../src/**), instead of trying to resolve it in complex way simply just copy cjs output to src allow spec resolves it.

most notably, now mocha runs via transpiled results so no more cache related problem should occur + runs faster as compilation only runs against changed files instead of running all files using ts-node.

@benlesh
Copy link
Member

benlesh commented Sep 25, 2019

cc @kolodny

- BREAKING CHANGES:

esm2015 renamed to esm. (though this is undocumented)
scoped package @reactivex/rxjs is being deprecated
@kwonoj
Copy link
Member Author

kwonoj commented Sep 25, 2019

What's being changed (potential breaking changes)

  • if you're explicitly traversing subpath esm2015, it is changed to esm: it is not supported to traverse subpath directly anyway
  • scoped package @reactivex/rxjs supposed to be depreacted, as this PR chanegs pkg manifest to use rxjs package name.

Package structure change

This is inner change, shouldn't create user-facing issue, but layout of structure has changed:

*Previous:
image

cjs was placed as top level, as well as we placed src by copying it. other build outputs are placed next to it as well.

*Changed:

image

Now dist contains all build outputs in its subpath, src stays on top as we package on repo root. subpath aliasing (ajax, operators...) are placed on top level as well to preserve current subpath import behavior.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM and it's so much faster 🏎

The dtslint tests fail for me, but I can look at that after this is merged.

@benlesh benlesh merged commit de6f1ef into ReactiveX:master Oct 15, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 14, 2019
@kwonoj kwonoj deleted the build-dist branch November 19, 2019 02:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants