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

Release prep #82

Merged
merged 4 commits into from
Apr 14, 2016
Merged

Release prep #82

merged 4 commits into from
Apr 14, 2016

Conversation

mustafaiman
Copy link
Contributor

This is mostly exposing both HazelcastClient and Config classes to users. Before this PR, we managed to use Config class in tests because we require it directly from lib folder by specifying the folder name. This is not desirable for end users.
With this PR users will be able to import

  • HazelcastClient as require('hazelcast-nodejs-client).Client
  • Config as require('hazelcast-nodejs-client').Config

This PR also adds "typings": "lib/index.d.ts" line to tsconfig.json. Typescript users will be able to use static type checking.

Adds "target": "es5" to tsconfig. Default was es3. Node.js fully supports es5. There is no compatibility issues with it.

@GhostInAMachine
Copy link
Contributor

Has conflicts

@GhostInAMachine
Copy link
Contributor

What exactly does using "es6-promise" achieve? It's just a watered down version of RSVP

@mustafaiman
Copy link
Contributor Author

@GhostInAMachine I am trying to decouple the Promise API from actual implementation. I thought of es6-promise as an API declaration. I may be wrong. Do you know how to instruct tsc compiler of users to use type definitions of ES6 Promises without depending on any specific implementation?

@GhostInAMachine
Copy link
Contributor

I'm not sure if ES6 promises will be an API to be implemented by third party solutions (like https://promisesaplus.com) or a thing to be included in ES6-compliant runtimes. I think it is the second.

https://tc39.github.io/ecma262/#sec-promise-objects

For the time being it is better not to concern ourselves too much with it.

@mustafaiman
Copy link
Contributor Author

If we provide API correctly, users are not restricted to use Q for their promises. I think this is a must because people will not want to introduce another promise library to their environment if they use Bluebird for example.

@GhostInAMachine
Copy link
Contributor

Good point, but the latest commit in this PR does not remove dependencies to Q.

Let's make it a goal for the next milestone.

@mustafaiman
Copy link
Contributor Author

It removes all dependencies to Q except one piece of API (Invocation). I am trying to remove it, too.

@GhostInAMachine
Copy link
Contributor

import * as Q from 'q';

How does this remove a dependency from Q?

@mustafaiman
Copy link
Contributor Author

We internally depend on Q for our promises. What I am doing is removing references to Q from our own public API. We should not expose anything specific to Q either in arguments list of our own public methods or return value of them.

@GhostInAMachine
Copy link
Contributor

Either way, I can't accept this PR with es6-promise, this is a rather weird solution to the problem.

@mustafaiman
Copy link
Contributor Author

I just bumped into microsoft/TypeScript#4168 (comment) . I do not like the solution either but I guess this is a common one.

@GhostInAMachine
Copy link
Contributor

Then perhaps we need to think of a better one.

@GhostInAMachine
Copy link
Contributor

Please remove last commit for this PR, let's discuss this problem more thoroughly. This holds back the rest of commits in this PR.

@mustafaiman
Copy link
Contributor Author

I removed the last commit but added a new one. New one changes module imports from deprecated import Q = require('q') syntax to import * as Q from 'q' syntax.

@GhostInAMachine GhostInAMachine merged commit f5212c9 into hazelcast:master Apr 14, 2016
@mustafaiman mustafaiman deleted the release-prep branch April 18, 2016 09:52
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