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

Merges into TKO #25

Open
brianmhunt opened this issue Jan 4, 2018 · 10 comments
Open

Merges into TKO #25

brianmhunt opened this issue Jan 4, 2018 · 10 comments

Comments

@brianmhunt
Copy link

Would you be interested in contributing some of this to tko / ko4?

@caseyWebb
Copy link
Contributor

caseyWebb commented Jan 4, 2018

absolutely. The utils and observable functions are probably the most useful.

@brianmhunt
Copy link
Author

brianmhunt commented Jan 4, 2018

Awesome, that's exactly what I was thinking. It shouldn't be hard; the one caveat is that I'd like to keep tko properly ES6 (i.e. not Typescript).

It might be a good test case for how we set up descriptors.

How do you think we should pick-and-choose what to merge?

@caseyWebb
Copy link
Contributor

The existing utils/observble.fn packages are all small and can easily have the types stripped out and we should be left with idiomatic ES6; we'll just have to ensure that they get added to DefinitelyTyped for us in TS-land.

I'm not sure what you mean by descriptors.

This is probably going to be pretty subjective. I'd target anything that

  • encourages best practices (like automatic cleanup using .subscribeOnce)
  • fixes a pain point that is frequently hit, like cleaning up or unwrapping "maybe observables" (.subscribeOnce again, soon-to-be-added bindings like ifEmpty and ifNotEmpty that prevent craziness like data-bind="if: ko.unwrap(foo).length === 0")
  • or just generally has a wide use case, and the util is unopinionated (.fromJS, .merge, etc.)

@brianmhunt
Copy link
Author

I agree. By descriptors, speaking as someone quite Typescript illiterate, the files that I gather go alongside the .js files that add type-checking to existing functionality. (Is that a thing? .ds files?)

@caseyWebb
Copy link
Contributor

caseyWebb commented Jan 5, 2018

Ahh, the declarations files, .d.ts. This would be the perfect time to figure out how to get those into TKO.

Having them alongside the source instead of in DefinitelyTyped could definitely help with making sure they get updated; things in DefinitelyTyped usually aren't updated right away. If you'd be willing to look at moving the tests — but not the source — to TS and using a hand written declaration file, then it'd become a lot more difficult to end up with declarations that don't match the source if you're updating the test every time you make a change to the module's exposed api.

@caseyWebb
Copy link
Contributor

@brianmhunt Would you like me to play around with tko and experiment with the best way to go about this?

I'm thinking utils.merge, utils.fromJS, and observable.fn.subscribeOnce are good candidates to begin with. Do you have any thoughts on observable.fn.increment and observable.fn.toString? toString could (should?) probably be added to the observable package directly.

@brianmhunt
Copy link
Author

@caseyWebb Thanks, yes feel free to play around.

I agree on the merge, fromJS. I think tko may have a once already, being (I expect) equivalent to subscribeOnce. My feelings on the name aren't strong, but I feel the functionality is pretty critical either way. Thoughts on naming? I think toString is in there, but not sure.

As for increment, I feel like we might want a more generic option e.g. a modify that takes e.g. a lambda:

>>> x = ko.observable(4)
>>> x.modify(y => y + 5)
9
>>> x()
9

Then increment would be trivial, or added as a curry. Internally modify (or whatever name makes sense) might something like:

modify (fn, peek) {  return this(fn(peek ? this.peek() : this())) }

Thoughts?

@caseyWebb
Copy link
Contributor

I wasn't aware of the new observable functions; I'm glad I looked because I was planning on implementing a few of those 😅 .

I like modify. That'd work out perfect.

@caseyWebb
Copy link
Contributor

@brianmhunt I'm coming back around to tko/knockout on the long arc of my todo list. I've started work on a branch in tko to set up TS typing. I'd like to discuss this with you a bit further before jumping in too deep, as there are two main setups that could be used while maintaining pure JS implementation.

The first is adding a single definition file to each package, e.g.

- src/
    - foo.js
    - bar.js
    - internal.js // used by foo and/or bar, but not exported
    - index.js // re-exports foo and bar
- index.d.ts // definitions for foo and bar

The second is to add ts definition files alongside each source file, e.g.

- src/
    - foo.js
    - foo.d.ts
    - bar.js
    - bar.d.ts
    - internal.js // used by foo and/or bar, but not exported
    - internal.d.ts
    - index.js // re-exports foo and bar
    - index.d.ts

Q: Why use the latter, more verbose, setup if the project isn't actually written in TypeScript?

A: You can enable basic type-checking in JS files (in VS Code) by adding // @ts-check to the top of the file. Similarly, you can run the tsc type checker on JS files with the allowJS option (this is already being used, as the build process uses typescript as the transpiler).

The modular setup would allow using the aformentioned // @ts-check directive to provide an additional layer of safety within tko/knockout itself, and not merely provide typings for consumers.

@brianmhunt
Copy link
Author

Thanks @caseyWebb. I like the latter approach, too - great suggestion.

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

No branches or pull requests

2 participants