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

refactor(package): change distribution to support main, module, es201… #2804

Merged
merged 2 commits into from Sep 17, 2017

Conversation

jasonaden
Copy link
Collaborator

…5 keys in package.json

The structure created by the build_all in this PR is different from the current one. There's now directories by language/package format. Everything gets built into dist as it already did, but then instead of publishing the cjs folder, we will now publish the package folder.

In here you will find folders for cjs, esm5, and esm15. The esmX folders represent ECMAScript Modules and the language level (es5 or es2015). cjs is exactly as it used to be (CommonJS format in es5 language level).

There is also a global folder (same as before) holding the UMD bundles. And a src directory with the TypeScript sources.

In order to support deep imports in all language levels, every file in the package has it's own package.json file that's dynamically generated in the .make-packages.js file.

@kwonoj
Copy link
Member

kwonoj commented Aug 21, 2017

We do have scoped package https://www.npmjs.com/package/@reactivex/rxjs for shipping all packges at once, while it currently doesn't have native ES module packages yet. (We did once and stopped, will do again with refined codes)

I don't think we'll going to change pkg publishing in rxjs. It was intentionally created for CJS only and it'll be.

@rxjs-bot
Copy link

rxjs-bot commented Aug 21, 2017

Fails
🚫

missing type definition import in tests (skipLast-spec.ts) (2)

Warnings
⚠️

❗ Big PR (1)

Messages
📖

(1) : Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.

(2) : It seems updated test cases uses test scheduler interface hot, cold but miss to import type signature for those.

CJS: 1289.7KB, global: 733.2KB (gzipped: 131.7KB), min: 162.5KB (gzipped: 31.3KB)

Generated by 🚫 dangerJS

@jasonaden
Copy link
Collaborator Author

@kwonoj So the point on this PR is to distribute different formats, while keeping support for deep imports. This way tools like Webpack will pick up the ESM files and will produce significantly smaller application bundles. CJS format is great for Node obviously, but Webpack and Rollup have a hard time optimizing it, therefore this format will allow both to co-exist.

I probably should have given a bit more background. I've been working with @benlesh to come up with some way to distribute Rx where we don't break existing applications (deep imports), but also supports optimizations through ESM format. This seems to be the solution that works.

@kwonoj
Copy link
Member

kwonoj commented Aug 21, 2017

thx for explanation. I'll leave this to @benlesh for primary review and will try to have some more context in core metings.

@jasonaden jasonaden force-pushed the publish_esm branch 3 times, most recently from b2ec014 to a383b56 Compare September 8, 2017 18:27
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

this seems correct to me, but I'll let Ben be the final judge since he's much more familiar with the codebase.

was this the package that I tested locally against the aio result of this build change? if not, we should probably test things on aio or similar before releasing it.

package.json Outdated
"build_closure_core": "Minify Global core build using closure compiler",
"build_global": "Build Global package, then minify build",
"build_perf": "Build CJS & Global build, run macro performance test",
"build_test": "Build CJS package & test spec, execute mocha test runner",
"build_cover": "Run lint to current code, build CJS & test spec, execute test coverage",
"build_docs": "Build ES6 & global package, create documentation using it",
"build_docs": "Build ESM15 & global package, create documentation using it",
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just use "2015" instead of "15" throughout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@benlesh We switched to esm2015 on the Angular project. Would you like to use the same?

@benlesh
Copy link
Member

benlesh commented Sep 17, 2017

@kwonoj I've worked with @jasonaden to try to get this PR right. The outcome is the best we can hope for with current technology stacks/build systems, I think. People should experience some decent code size reductions if they're using Rollup or Webpack properly with this new way of distributing the code.

Merging this (despite the dangerjs complaints) ahead of the 5.5 release.

@benlesh benlesh merged commit 988e1af into ReactiveX:master Sep 17, 2017
@kwonoj
Copy link
Member

kwonoj commented Sep 17, 2017

@benlesh yeah I'm fine, 👍

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
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.

None yet

5 participants