V12 #151

thgreasi · 2020-08-13T23:45:52Z

Convert to typescript and publish typings
Add a required baseUrl parameter
Default refreshToken to true only when the target provided request.send baseUrl/url option matches the default baseUrl option passed to the factory function.
change the refreshToken endpoint to /user/v1/refresh-token

See: https://www.flowdock.com/app/rulemotion/resin-frontend/threads/XVvB7BhMFMXAXsQNwACFg5seFyQ

Page- · 2020-08-14T08:29:45Z

lib/request.ts

 		if (options == null) {
-			options = {};
+			options = {} as BalenaRequestOptions;


This should not need to be cast, if options is actually optional and therefore every property is actually optional then the types should reflect that and not need casting. In fairness though I wouldn't be surprised if it's actually required and this defaulting should be removed

So as part of this commit I just converted to TS, but I agree on removing all such defaulting so that we can get rid of this weird casting.

Page- · 2020-08-14T08:30:44Z

tsconfig.json

@@ -1,7 +1,7 @@
 {
    "compilerOptions": {
        "module": "commonjs",
-        "declaration": false,
+        "declaration": true,
        "outDir": "build",
        "noImplicitAny": false,


If you've converted it all to typescript then you can set this to true

Suggested change

"noImplicitAny": false,

"noImplicitAny": true,

and also remove the checkJs/allowJs below

Going for "strict": true, instead since it enables this as well.

Page- · 2020-08-14T08:31:06Z

package.json

+    "lint": "balena-lint -e js -e ts --typescript lib",
+    "lint-fix": "balena-lint -e js -e ts --typescript --fix lib",


Is there still any js? If not you can remove the extension args altogether

Page- · 2020-08-14T08:33:52Z

lib/request.ts

+		return exports.interceptors
+			.slice()
+			.reverse()
+			.reduce(function (promise, { response, responseError }) {
+				if (response != null || responseError != null) {
+					return promise.then(response, responseError);
+				} else {
+					return promise;
+				}
+			}, initialPromise);


If you're able to simplify this reduce promise chain building usign async/await I would love that

Let me try that as a minor after we gather all the breaking changes.

Page- · 2020-08-14T08:36:10Z

lib/request.ts

+		return exports.interceptors
+			.slice()
+			.reverse()


I think it'd be worth thinking about making the exposed interface be addInterceptor: (interceptor: Interceptor) => void instead of interceptions: Interceptor[], that way internally addInterceptor can do interceptors.unshift(interceptor) and we can avoid the need to clone/reverse the array for every request and therefore reduce GC pressure

I like the idea but this would require a breaking change probably in the SDK, and we are doing major releases often enough there lately 🤔

Page- · 2020-08-14T08:38:01Z

lib/request.ts

@@ -328,7 +388,6 @@ export function getRequest({
 	 * 		throw error
 	 * )
 	 */
-	exports.interceptors = interceptors;


All the docs for inteceptors are directly above this, I think you'll need to move the docs somewhere else now for it to all format/generate correctly

Had some trouble to get this right but eventually I had to take a shortcut. Hope you are fine with it :)
Fun fact, the doc generation was already broken on master...

Page- · 2020-08-14T08:38:46Z

lib/request.ts

+	async function refreshToken({
+		baseUrl,
+	}: Pick<BalenaRequestOptions, 'baseUrl'>): Promise<string> {


There is a question to be asked here as to whether "balena-request" should know about refreshing tokens or whether that should actually be moved into "balena-auth" instead?

I wasn't part of the discussion when this was added, but I'm guessing that the reason might be that balena-auth doesn't have access to the balena-request instance, but it's the other way around.
And since balena-request atm does refresh the token when before doing a request when the token is old enough, so we will have to think about a way that we don't have a weird circular dependency schema.

Page- · 2020-08-14T08:39:52Z

lib/progress.ts

 		if (requestAsync == null) {
 			requestAsync = utils.getRequestAsync();
 		}


I think it's worth converting all these nullish defaults to es6 defaults instead since we're doing a breaking

Page- · 2020-08-14T08:42:35Z

lib/progress.ts

@@ -105,13 +110,13 @@ export function estimate(requestAsync, isBrowser) {

 		const stream = require('stream');


Can you go through and make sure all requires like this have an added as typeof import... - in javascript the require call itself is typed when possible but in typescript you have to explicitly type it, meaning with the conversion we'll actually have lost typings in some places

Change-type: squash Signed-off-by: Thodoris Greasidis <[email protected]>

Resolves: #96 Change-type: major Signed-off-by: Thodoris Greasidis <[email protected]>

thgreasi self-assigned this Aug 13, 2020

Page- reviewed Aug 14, 2020

View reviewed changes

thgreasi requested a review from Page- August 14, 2020 12:48

thgreasi mentioned this pull request Aug 28, 2020

Add missing balena-request typings for the refreshToken method balena-io/balena-sdk#982

Merged

4 tasks

thgreasi force-pushed the v12 branch from ecf3878 to 57e35be Compare February 9, 2021 18:18

thgreasi force-pushed the v12 branch from 57e35be to 12fd8c0 Compare March 12, 2021 17:00

thgreasi added 3 commits November 28, 2021 15:38

Stop defaulting nullish options in the prepareOptions()

7124efe

Change-type: squash Signed-off-by: Thodoris Greasidis <[email protected]>

Use es6 defaults when possible

d9570f4

Change-type: squash Signed-off-by: Thodoris Greasidis <[email protected]>

Make the baseUrl parameter required for refreshToken()

332b26b

Resolves: #96 Change-type: major Signed-off-by: Thodoris Greasidis <[email protected]>

thgreasi force-pushed the v12 branch from 12fd8c0 to 332b26b Compare November 28, 2021 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V12 #151

V12 #151

thgreasi commented Aug 13, 2020 •

edited

Loading

Page- Aug 14, 2020

thgreasi Aug 14, 2020

Page- Aug 14, 2020

thgreasi Aug 14, 2020

Page- Aug 14, 2020

Page- Aug 14, 2020

thgreasi Aug 14, 2020

Page- Aug 14, 2020

thgreasi Aug 14, 2020

Page- Aug 14, 2020

thgreasi Aug 14, 2020

Page- Aug 14, 2020

thgreasi Aug 14, 2020

Page- Aug 14, 2020

thgreasi Aug 14, 2020 •

edited

Loading

Page- Aug 14, 2020

		"lint": "balena-lint -e js -e ts --typescript lib",
		"lint-fix": "balena-lint -e js -e ts --typescript --fix lib",

		@@ -105,13 +110,13 @@ export function estimate(requestAsync, isBrowser) {

		const stream = require('stream');

V12 #151

Are you sure you want to change the base?

V12 #151

Conversation

thgreasi commented Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thgreasi Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thgreasi commented Aug 13, 2020 •

edited

Loading

thgreasi Aug 14, 2020 •

edited

Loading