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

Remove redundant dependencies #1830

Merged
merged 6 commits into from
Jan 16, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/sdk-auth/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
},
"dependencies": {
"@commercetools/sdk-middleware-http": "^7.0.0",
"lodash.defaultsdeep": "^4.6.0",
"lodash.defaultsdeep": "^4.6.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this dependency still needed after the change to Object.assign?

Copy link
Contributor Author

@ajimae ajimae Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just removed it 🙂

"qss": "2.0.3"
},
"devDependencies": {
Expand Down
22 changes: 16 additions & 6 deletions packages/sdk-auth/src/auth.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
/* @flow */

import type {
AuthOptions,
CustomAuthOptions,
Expand All @@ -9,7 +8,6 @@ import type {
UserAuthOptions,
} from 'types/sdk'
import { decode, encode } from 'qss'
import defaultsDeep from 'lodash.defaultsdeep'
import { getErrorByCode } from '@commercetools/sdk-middleware-http'
import * as constants from './constants'

Expand Down Expand Up @@ -88,7 +86,8 @@ export default class SdkAuth {
clientId,
clientSecret,
}: ClientAuthOptions): string {
return Buffer.from(`${clientId}:${clientSecret}`).toString('base64')
const targetStr = `${clientId}:${clientSecret}`
return typeof Buffer === 'undefined' ? btoa(targetStr) : Buffer.from(targetStr).toString('base64')
}

static _getScopes(scopes: ?Array<string>, projectKey: ?string): string {
Expand Down Expand Up @@ -221,7 +220,7 @@ export default class SdkAuth {
const { uri, body, basicAuth, authType, headers } = request
const fetchHeaders = headers || {
Authorization: `${authType || constants.DEFAULT_AUTH_TYPE} ${basicAuth}`,
'Content-Length': Buffer.byteLength(body).toString(),
'Content-Length': JSON.stringify(body?.length || 0),
'Content-Type': 'application/x-www-form-urlencoded',
}

Expand All @@ -237,10 +236,21 @@ export default class SdkAuth {
}

_getRequestConfig(config: CustomAuthOptions = {}): AuthOptions {
const mergedConfig = defaultsDeep({}, config, this.config)

// handle scopes array - defaultsDeep would merge arrays together
// It is important to note the difference between plain old javascript Object.assign()
// and the lodash _.defaultsDeep() functions especially how they are both used here

// _.defaultsDeep({ 'a': { 'b': 2 } }, { 'a': { 'b': 1, 'c': 3 } }) // { a: { b: 2, c: 3 } }
// Object.assign({ 'a': { 'b': 2 } }, { 'a': { 'b': 1, 'c': 3 } }) // { a: { b: 1, c: 3 } }

// handle scopes array - Object.assign would merge arrays together
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is actually not really good if a missing dependency would change the behavior here. I would take a look what's the size of the config and do the merge straightforward in a function without the need of any dependency. Or directly use Object.assign

// instead of taking its first occurrence
// const mergedConfig = defaultsDeep({}, config, this.config)

const mergedConfig = typeof window === 'undefined' ?
// eslint-disable-next-line global-require, prefer-object-spread
require('lodash.defaultsdeep')({}, config, this.config) : Object.assign({}, this.config, config)

if (config.scopes) mergedConfig.scopes = config.scopes

return mergedConfig
Expand Down
16 changes: 16 additions & 0 deletions packages/sdk-auth/test/token-provider.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -342,4 +342,20 @@ describe('Token Provider', () => {
expect(_tokenProvider.tokenInfo).toEqual(null)
})
})

describe('scopes', () => {
const auth = new Auth(config)
test('should include scope in the merged config', () => {
expect(config.scopes).toBeFalsy()

const mergedConfig = auth._getRequestConfig({ scopes: ['new-scope:demo-projectKey'] })

expect(mergedConfig).toBeTruthy()
expect(mergedConfig).toMatchObject(
expect.objectContaining({
scopes: ['new-scope:demo-projectKey']
})
)
})
})
})
Loading