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: refactor build script #4861

Closed
wants to merge 1 commit into from

Conversation

filipesilva
Copy link
Contributor

Following a conversation with @benlesh, I'm taking a look at the tooling in RxJS to see where things can be simplified or removed.

I wanted to put this PR while it's still in progress to see if I'm missing important things or changing stuff I really shouldn't.

With the current changes, the dist/ folder has a slightly different layout:

/dist
├── /ajax
|  ├── LICENSE.txt
|  └── package.json
├── /fetch
|  ├── LICENSE.txt
|  └── package.json
├── /operators
|  ├── LICENSE.txt
|  └── package.json
├── /testing
|  ├── LICENSE.txt
|  └── package.json
├──/webSocket
|  ├── LICENSE.txt
|  └── package.json
├── /formats
|  ├── /cjs
|  ├── /esm2015
|  ├── /esm5
|  └── /typings
├── /bundles
|  ├── rxjs.umd.js
|  └── rxjs.umd.min.js
├── LICENSE.txt
├── package.json
└── README.md

The ajax, fetch, operators, testing, and webSocket folders remain mostly the same. They are secondary entry points and just point to the format specific files. There's no index.js in them anymore though.

{
  "name": "rxjs/ajax",
  "main": "../formats/cjs/ajax/index.js",
  "module": "../formats/esm5/ajax/index.js",
  "es2015": "../formats/esm2015/ajax/index.js",
  "typings": "../formats/typings/ajax/index.d.ts",
  "sideEffects": false
}

The formats folder contains all the different supported formats: cjs, esm2015, esm5, and typings. The UMD bundles are inside bundles. Maybe they should also be inside formats but since users do deep imports for them today, I left them in the same place.

Then there's the license, package.json and readme in the top level.

I think this format still caters to all the same consumers are before, but is simpler and requires moving less stuff around. Along the way I tried to remove scripts that weren't needed anymore. .make-packages.js is now much smaller and does all the build/packaging tasks.

I still have more scripts to look at and try to simplify but this is a start.

@benlesh (and others) please let me know what you think.

// TODO:
// - what about sourcemaps?
// Angular itself has inline sourcemaps. I don't see the point, since you don't have sources
// on the package.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what to do about sourcemaps. I think the current ones are a bit weird and work only because the TS sourcefiles are bundled. We could inline sourcemaps but I'm not sure if that impacts parsing performance.

// Angular itself has inline sourcemaps. I don't see the point, since you don't have sources
// on the package.
// - what about shipping TS for bazel support?
// I think this isn't needed anymore?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does RxJS still need to ship TS for Bazel support? I thought this wasn't needed anymore.

// Run BO over esm5 and esm2015 to remove side effects.
console.log('# Removing side effects from esm5 and esm2015.');
const buildOptimizerOptions = { emitSourceMap: false, isSideEffectFree: true };
ls('./dist/formats/{esm5,esm2015}/**/*.js').forEach(inputFilePath => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running Build Optimizer over both ESM5 and ESM2015 should enable tree shaking for both (and prevent rollup/rollup#2415).

@benlesh
Copy link
Member

benlesh commented Jun 30, 2020

Hey, @filipesilva, this is old, and probably fell through the cracks when I left Google. I appreciate the effort, but I think we've got other solutions in place for now. Is it okay if we close this one? Or do you still see opportunity to improve what we have?

@filipesilva
Copy link
Contributor Author

SGTM Ben, no worries at all.

@filipesilva filipesilva closed this Jul 1, 2020
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.

None yet

2 participants