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

'Knockback' and 'kb' are not interchangeable #146

Closed
vincentw56 opened this issue Sep 19, 2015 · 14 comments
Closed

'Knockback' and 'kb' are not interchangeable #146

vincentw56 opened this issue Sep 19, 2015 · 14 comments

Comments

@vincentw56
Copy link

According to the documentation, 'Knockback' and 'kb' are interchangeable which is not the case. The issue is that when using this with Typescript, it causes issues after a build because it uses 'Knockback' in some places. I have to go in to the compiled JS and replace the words.

@vincentw56 vincentw56 changed the title 'Knockback' and 'kb' 'Knockback' and 'kb' are not interchangeable Sep 19, 2015
@kmalakoff
Copy link
Owner

@vincentw56 sorry to hear. I'm not familiar with TypeScript so I'll need a little help figuring out what the problem is and what to do about it...

Personally, I've never used the word Knockback when using the library and I believe it was just a copy onto window a few years ago, but it might no longer be supported since I moved over the library packaging to webpack around a year ago.

Should I update the documentation to not refer to Knockback or is this a bug related to TypeScript that can be solved (if so, how)?

@vincentw56
Copy link
Author

If it isn't support any more, I would suggest removing it from the documentation. It isn't really a bug with Typescript. It is more along the lines of how the typings file interprets it. I will see if I can get that file updated which should fix it. I didn't want to do that if Knockback was supposed to support both keywords.

@abradley2
Copy link

@vincentw56 Do you have a .tds file/ typescript definitions for Knockback?

@vincentw56
Copy link
Author

@abradley2 Yes, I use the one from here: https://github.com/borisyankov/DefinitelyTyped. When I use this code:

class UserViewModel extends Knockback.ViewModel {
}

Typescript compiles it to:

var UserViewModel = (function (_super) {
__extends(UserViewModel, _super);
function UserViewModel() {
_super.apply(this, arguments);
}
return UserViewModel;
})(Knockback.ViewModel);

You can see it keeps the "Knockback.ViewModel" which isn't supported. If I change it to "kb.ViewModel" it works fine. The issue is that the Typescript typings file doesn't support "kb.ViewModel".

@kmalakoff
Copy link
Owner

Wow! I had no idea someone had made the .ts file.

I'll deprecate Knockback and will replace with kb in the next release. It was one of those things where I tried to straddle two worlds (Backbone -> Knockback and ko -> kb).

I'm happy to learn TypeScript since it is heating up and to add some tests. Not sure when I will get to it, but it is on my list now. Any help or pointers for how to do this would be much appreciated!

@ghost
Copy link

ghost commented Jan 31, 2017

@vincentw56

Do you have a work around for this?

I tried creating a "patch" definition but unfortunately you can't retype a variable (which makes sense).
I'm rather loathed to make my own version of the definition file.

@vincentw56
Copy link
Author

@AndyCJ I put this bit of code in that did the trick:

var Knockback = kb;

I've since moved on to Aurelia.

@ghost
Copy link

ghost commented Feb 1, 2017

@vincentw56 Thanks for that. I got that working when using kb in an ambient/global scope, but unfortunately I still couldn't get it working when using RequireJS/AMD.

Despite saying I don't want to maintain my own type definition I have gone ahead and started to do that as it fixes all these problems at the root.

I'll try submitting a pull request to definitely typed if I think the end result meets their quality requirements.

@kmalakoff
Copy link
Owner

I've been following this discussion, but because I still haven't started playing around with TypeScript, I'm not sure how I should help. Perhaps you can take the type definitions file and replace all instances of Knockback with kb?

If you would like me to deprecate Knockback and only publish kb, I could.

@ghost
Copy link

ghost commented Feb 2, 2017

Having Knockback be a "true" alias of kb would help, but the TS definition would still be broken, and that's the root of the issue.

For example it's not set to work as an AMD module definition either, and the only way to fix that is to change the definition itself.

Here's my updated TS definition: https://gist.github.com/AndyCJ/89c57d98f5de33d3fcb1f32bb9e08b5c

I've only tried it out loading knockback as an AMD module and creating a few simple ViewModels so I'm sure there's further bits to be fixing.

But it is better than the existing offering: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/knockback/index.d.ts

@kmalakoff
Copy link
Owner

Not sure if there should be an alias all. To keep things simple, I'd probably rather only export one symbol kb since it is more what people expect from a library.

What would you think about that as a solution instead? eg. remove my over-engineering by simplifying

@ghost
Copy link

ghost commented Feb 3, 2017

That sounds like a good idea, I don't think you'll lose much by getting rid of it.

The only worry I'd have is if you have lots of devs already using the alias, but the documentation and reference app all use kb so I'd hope it's a small number using Knockback.

@kmalakoff
Copy link
Owner

Agreed. I would need to update the docs, tests, and bump a major version. I'll do this over the weekend.

Plus someone would need to update the types file.

@kmalakoff
Copy link
Owner

OK. I've looked into this. I think the aliasing of kb to Knockback was removed when we switched the build system over to webpack a couple of years ago.

So it looks like I just need to update the documentation and the person responsible for the type definitions needs to replace Knockback with kb.

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

3 participants