Skip to content

Conversation

@mrozbarry
Copy link

Summary

  • Use NodeJS 10.16 LTS
  • Add .env.example to make it easy to copy/paste in a key without causing an issue
  • Ignore .env along with .env.example
  • Extract Spotify API and SDK Calls into a separate module
  • Add tests

@@ -0,0 +1,60 @@
/* global Spotify */
Copy link
Author

Choose a reason for hiding this comment

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

Isolate API and SDK side-effects for easier testing

const original = axios.defaults.adapter
axios.defaults.adapter = request

const configWith = options => ({
Copy link
Author

Choose a reason for hiding this comment

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

Not a huge fan of this - axios has a handful of default request configuration options which made it difficult to do a direct assertion on what the config looked like as a request.

Now that I'm looking at this, we could just record a collection of the config objects, which would make it easier to do something like expect(config[0].url).toBe('...') rather than abuse jest mock functions.

)
this.player.addListener('ready', ({device_id}) => {
this.player.on('ready', ({device_id}) => {
Copy link
Author

Choose a reason for hiding this comment

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

Looks like addListener and on do the same thing, so I switch this to make my test mocking easier, and to make this block more consistent.

}
});
SpotifyService.shuffle(deviceId, state, this.token.token)
.catch(console.error) // eslint-disable-line no-console
Copy link
Author

Choose a reason for hiding this comment

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

This will need to be more robust. The main cases here are:

  1. No internet (status code 0?); probably show a warning/error
  2. Expired token (status code 401); open a popup, do the auth flow
  3. Spotify API is out (status code >= 400) ; maybe same flow as 1
  4. We introduced a bug (exception); maybe same flow as 1

We could put that logic in a service module, since there may be a number of places where we need to handle API errors.

"@vue/cli-plugin-unit-jest": "^3.10.0",
"@vue/cli-service": "^3.8.0",
"@vue/test-utils": "^1.0.0-beta.29",
"babel-core": "7.0.0-bridge.0",
Copy link
Author

Choose a reason for hiding this comment

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

The vue cli added this dep for me, but I am pretty sure recent jest versions don't need this any more.

/dist

# local env files
.env
Copy link
Owner

Choose a reason for hiding this comment

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

I believe all the contents of this .gitignore are what were generated when Vue CLI created the app. Strange it didn't include .env

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