applitools / Eyes.Sdk.JavaScript

Applitools SDK for Selenium 3. This repository is deprecated. It has moved to https://github.com/applitools/eyes.sdk.javascript1/tree/master/packages/eyes-selenium-3
6 stars 17 forks source link

Needlessly passing PromiseFactory to multiple object creations #54

Closed corevo closed 5 years ago

corevo commented 6 years ago

Every object constructed with the SDK requires as a last argument a PromiseFactory.
Since mostly this is determined by the environment rather than by the object's implementation, most likely these factories are going to be identical in said environment.
Meaning having a reference to it and passing it every time becomes a chore, to overcome that, I suggest setting a global Promise Factory, and unless given another one in the object's ctor, assume that.
Moreover in Node or Browser environment, where none is given, a default one can be assumed, as long as a global Promise is defined.

export const promiseFactory = {
  makePromise: (p) => (new Promise(p)),
  resolve: Promise.resolve.bind(Promise),
  reject: Promise.reject.bind(Promise)
};
astappiev commented 6 years ago

The default PromiseFactory is already introduced in eyes.sdk.core and its successors.

But I would like to avoid using any global variables. We don't know how different customers use the global environment. Maybe they use their own promiseFactory, and probably, it can be not compatible with ours. If there is another solution without using globals, we should do that.

corevo commented 6 years ago

I definitely think that if the user didn't pass in to the ctor a PromiseFactory, we can definitely use global.Promise if it is defined.
Especially since all versions of Node in which there is no global.Promise have reached their EOL.
Also about using global variables, I wasn't suggesting placing anything on the global object, that is definitely bad practice, but surely a PromiseFactory module based on global.Promise can be shipped along side the SDK, that will be lazy-ly loaded in case the user didn't ship his own factory.

I am saying all of this, because I find it very weird that the user has got to know the SDKs internals, and pass an internal factory to the SDK by default, other I/O node libraries either ship with their own default implementation (such as mongo), or use the default Promise object.

astappiev commented 6 years ago

I definitely think that if the user didn't pass in to the ctor a PromiseFactory, we can definitely use global.Promise if it is defined.

Yes, I agree with that. I already did it for eyes.selenium https://github.com/applitools/Eyes.Selenium.JavaScript/pull/61/commits/76e1736b46a8f757709c1b8f3e35bde74f5c9823

Also, you can try to use @applitools/eyes.sdk.core package. In next few weeks I will update it and new version will not include promiseFactory at all, we are going to replace them with async/await similar to new Selenium 4 SDK. But in the current version, there is default ctror for it.

astappiev commented 6 years ago

This commit https://github.com/applitools/Eyes.Sdk.JavaScript/pull/60/commits/d7b7659f0bc74d3dfaf2a886e7c358eaf922aa30 will add default paramets to ctor, after @danielputerman merge it.