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

Add customer route #1

Merged
merged 16 commits into from
Aug 21, 2020
Merged

Add customer route #1

merged 16 commits into from
Aug 21, 2020

Conversation

hibes
Copy link
Contributor

@hibes hibes commented Aug 10, 2020

No description provided.

Copy link

@geoff-sheldon geoff-sheldon left a comment

Choose a reason for hiding this comment

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

LGTM, but I do have a couple weird questions

@@ -0,0 +1,4 @@
import config from '../config';

Choose a reason for hiding this comment

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

How does JS determine which config to use? Or does this just pass in the contents of all of the .js and .json files in the config folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Javascript, when you don't specify a specific js file, defaults to the index.js file.

The logic for selecting configuration values is here: https://github.com/0time/npcrm-client/blob/master/config/index.js

It basically just starts with default.js, shoves in the ${process.env.NODE_ENV} config file, and then overrides with anything in local.json.

'config.apiServer.basePath',
)}`,
},
get(context, 'config.defaultAxiosArgs', {}),

Choose a reason for hiding this comment

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

When you pass arguments into query, do they get placed in this empty object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On line 17, the empty object is a default parameter if the json path (config.defaultAxiosArgs) does not exist.

But yeah, this instance object has default Axios parameters defined by axios.create({...}), so any calls to query.js will default with those parameters.

If you pass any other object parameters to the query.js call, those should override the defaults, assuming axios implemented these features the way I would expect.

@@ -0,0 +1 @@
export default (content) => console.error(content) || content;
Copy link

Choose a reason for hiding this comment

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

I'm a little confused by this one. Does this function as an OR using some behavior of console.error I'm not familiar with, or does it just do both?

edit: The reason I'm confused is because it seems like this would always short circuit, as long as content isn't a falsey value - in my head, this means you'd always log an error. I did some digging on console.error and it looks like it'll accept any string input - so is this basically saying the following Ruby-ish pseudocode?

if content.is_a?(String) ? log_error : return(content)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this is just bad code. I'll fix it.

console.error, console.log, etc. return a falsy value (undefined if I recall correctly), so this is just a lazy way of writing to both log and return the content variable.

Copy link

@geoff-sheldon geoff-sheldon left a comment

Choose a reason for hiding this comment

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

lgtm 🎉

@hibes hibes merged commit c3619da into master Aug 21, 2020
@hibes hibes deleted the add-customer-route branch August 21, 2020 18:03
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.

None yet

2 participants