vow promises -> native Promise #11
Conversation
jifeon
changed the title
| * @returns {string} | ||
| * @private | ||
| */ | ||
| function getRedirectUrl(currentUrl, redirectUri) { |
There was a problem hiding this comment.
Why second param ends with i and first with l. Shall it be the same Url ending?
There was a problem hiding this comment.
signature was not changed, PR is about promises at most. It initially was written by you, so the question to you lol 🐗
| const deferred = vow.defer(), | ||
| page = this._page, | ||
| args = Array.prototype.slice.call(arguments, 0); | ||
| evaluateJs(...args) { |
There was a problem hiding this comment.
Would be very helpful if you can state here JSDoc with an example how to call this function
There was a problem hiding this comment.
it was not changed, but I agree jsdoc is needed. I added a point into #1 since I think docs should be fixed in separate PR
|
|
||
| this._requestingActions.push({ | ||
| pattern: uri, | ||
| fn(err, results) { |
There was a problem hiding this comment.
Just in case you can clarify this line: don't see any fn defined in surrounding (in the old code as well)
There was a problem hiding this comment.
But it's the declaration itself. Strange that it's not an arrow function. But still PR is not about refactoring everything, so I would prefer to fix it in next PRs.
const a = {
fn: function() {}
}is the same as
const a = {
fn() {}
};
| } | ||
|
|
||
| injectBrowserEnv() { | ||
| if (this._browserEnvInjected) { | ||
| return vow.resolve(); | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| debug('.inject()-ing browser env libs'); |
There was a problem hiding this comment.
Here injects browser.bundle.js before it was build via webpack. But now - not, I think this is a browser build of goose-parser. We need to inject it in different way (for example by passing from the point where we define parser and env). Also starting from that _browserEnvInjected meaningless.
There was a problem hiding this comment.
Agree, it's broken now by moving env into repo (not by this PR). I will create a separate issue for it.
jifeon
changed the title
Also:
Required before merge
closes #6
ref #1