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

Legacy browser support, better Monorepo usage #39

Merged
merged 20 commits into from
Oct 23, 2017

Conversation

caseyWebb
Copy link
Contributor

@caseyWebb caseyWebb commented Oct 15, 2017

This is a first, although pretty comprehensive, pass at refactoring the monorepo that...

  • uses lerna more idiomatically
  • adds transpilation via tsc (TypeScript compiler) for legacy browser support (down to ES3)
  • adds .es6, .min, and .es6.min builds
  • (kind of) fixes the tests

Idiomatic Lerna

The built browser bundle has been moved into the packages directory. Instead of using rollup-plugin-includepaths, packages are linked via package.json => dependencies (and devDependencies for test-only imports), which lerna bootstrap then does its magic on. lerna bootstrap has been registered as a postinstall hook at the root level, so installing deps and linking packages across the entire repo can be done with a yarn install in the root.

Legacy Browser Support

Instead of Babel, the TypeScript compiler is used with the allowJs option and a target of ES3. It's faster, compiles down lower than babel (afaik, babel only supports ES5), and will allow for incremental adoption of TypeScript if desired.

Multiple Builds

The browser bundle has 4 builds available: ES3, ES6, ES3+Minification, ES6+Minification.
Individual modules do not have minified builds, because they can only be consumed via JS bundlers and minification should be done last for best results (not to mention debugability).

Modular Usage

TKO modules are passed through rollup, but other TKO modules are marked as external to prevent a bunch of duplication and are output in ES2015 module format and thus only supported with modern JS bundlers. Rolling them up renders faster builds for consumers wish to use individual modules, and allows transpilation without bloating the build process. Theoretically, we could create AMD/SystemJS bundles as well for script tag usage, but if you're going through the hassle of optimizing for weight, you're probably using ES2015 modules — at least you should be, IMHO of course.

The combined bundle on the other hand uses UMD format for universal consumption. AFAIK, tree-shaking won't take effect even if this is output in ES2015 module format without some refactoring, so this seems most reasonable for the moment.

Documentation for modular usage is very much needed. Currently the only way I could get it working was copying packages/tko/src/index.js to my own project and removing the parts I don't need.

Tests

Jasmine was upgraded to an incompatible version at some point. This has been rolled back, so now the test failures are actual failures.

@caseyWebb
Copy link
Contributor Author

caseyWebb commented Oct 15, 2017

x-ref #6
x-ref #21

@brianmhunt
Copy link
Member

Awesome! Looking forward to digging into this — will dig into it and comment this week. Thanks @caseyWebb !

@brianmhunt
Copy link
Member

This is excellent. There's a lot of much needed cleanup in here.

I was thinking of moving the tko dist to packages/tko too, so I'm glad you did that.

In general, tko is going to be a "reference implementation" of the modules, and not necessarily for export itself. So copying it and modifying it for your needs is its essential purpose, though it'd be nice if there were an e.g. "tko.builder" package that brought all the pieces together in a more homogenous way. The modularity documentation will go with the tko package.

The knockout package will stick to more conservative guidelines, though I haven't nailed down the exact delineation between Knockout and the tko reference implementation yet.

I think we can remove ~/.gitattributes, since git-lfs is really more a pain than it's worth in this context.

The tests are written for Jasmine version 1.3, which is incompatible with Jasmine 2.0. Jasmine 1.3 will be here for a long time; we use mocha+chai+sinon for some of the newer packages.

What's the difference between npm run build and yarn build?

The tests (npm test) aren't working for me; b/c of

19 10 2017 15:29:11.093:ERROR [preprocessor.rollup]: Error processing “spec/extenderBehaviors.js”

Cannot read property 'slice' of undefined

You need to include some adapter that implements karma.start method

Are you experiencing this too? Did you have thoughts on fixing them?

This is a pretty hefty commit but it's the way to go, so I'm ok merging this and fixing up anything it breaks, unless there's something that stands out as needing attention before that should happen.

Thanks @caseyWebb — This is a much needed and valuable patch!

@caseyWebb
Copy link
Contributor Author

A tko.builder would be superb. I'd be interested in helping design an API based on what is happening in tko.

+1 on git-lfs. I actually ran into an issue cloning the repo because I have an ssh alias for github which git-lfs couldn't resolve, and had to change the remote from gh:knockout/tko to the much more verbose git@github.com:knockout/tko. One of those things that's probably only an issue for a handful of people (or just me), but left me with a bad taste towards git-lfs in general.

As far as npm run vs yarn, it's exactly the same, but yarn allows omitting the run part of the command. Works with it, but works without it all the same. IMO, it's a bit less explicit, but more idiomatic. I only swapped it out in the docs to further emphasize that this project is based on yarn. I'm a fan of all-or-nothing usage for simplicity's sake.

Most of the tests pass for me, but there are still some errors. I'll have to double check to see what the errors are, but I don't believe it's those. To be honest, I'd really like to see a migration to jest instead of mocha if migrating the tests is a goal. It's extremely fast, and based on jasmine already.

There is definitely a lot going on in here. I began with trying to restrict changes to just the build, but revamping the lerna usage made that a whole lot easier and less overall code, so it seemed the right way to go.

AFAIK, there is nothing that needs to be done before merging this.

@brianmhunt brianmhunt merged commit eb8219a into knockout:master Oct 23, 2017
@brianmhunt
Copy link
Member

Awesome, thanks for the comment @caseyWebb

Let's set up new issues and reference them:

  1. start tko.builder
  2. remove git-lfs
  3. fix up testing
  4. evaluate jest as an alt. test system

Just a quick merge & comment right now; I'm hoping to set aside some time for this soon, but feel free to get started with the issues.

@caseyWebb caseyWebb deleted the legacy-browser-support branch October 23, 2017 18:49
@caseyWebb caseyWebb mentioned this pull request Nov 13, 2017
@brianmhunt brianmhunt mentioned this pull request Jan 19, 2018
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.

2 participants