Closed caseyWebb closed 7 years ago
x-ref #6 x-ref #21
Awesome! Looking forward to digging into this — will dig into it and comment this week. Thanks @caseyWebb !
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!
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.
Awesome, thanks for the comment @caseyWebb
Let's set up new issues and reference them:
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.
This is a first, although pretty comprehensive, pass at refactoring the monorepo that...
Idiomatic Lerna
The built browser bundle has been moved into the
packages
directory. Instead of usingrollup-plugin-includepaths
, packages are linked viapackage.json => dependencies
(anddevDependencies
for test-only imports), whichlerna 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 ayarn install
in the root.Legacy Browser Support
Instead of Babel, the TypeScript compiler is used with the
allowJs
option and a target ofES3
. 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.