Skip to content

Helpers for config and API client #1452

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

Merged
merged 3 commits into from
Jun 15, 2020

Conversation

andrewn
Copy link
Member

@andrewn andrewn commented Jun 8, 2020

I'm trying to add Storybook stories for the FileNode component but this isn't possible because it imports Redux actions which rely on reading window.process.env, which throws an error in the storybook environment. 😢

I noticed that the code for reading the env var is duplicated throughout the app, so I've refactored into a helper called getClient(key). This also abstracts where the var is being read from, so in future we can read it from elsewhere, or try a few different places (local storage, then the env) without changing the calling code.

The biggest user of the env var is the API_URL in the redux actions. I've created apiClient which is an instance of axios that provides a single place to configure the API_URL and basic config that we want to apply to all communication to the editor API.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)
  • is descriptively named and links to an issue number, i.e. Fixes #123

andrewn added 2 commits June 8, 2020 11:46
Reduces the amount of duplication and provides a single place where
we can configure base URL, crendentials and other headers
@catarak
Copy link
Member

catarak commented Jun 10, 2020

Thank you! This is awesome and cleans up the code a lot. One thing I'm noticing is that when I run the tests, I'm getting some warnings about the config variables being undefined:
Screen Shot 2020-06-10 at 5 00 25 PM
I'm seeing these warnings in Nav.test.jsx, getConfig.test.js, FileNode.test.jsx, and Toolbar.test.jsx.

@andrewn
Copy link
Member Author

andrewn commented Jun 13, 2020

I've changed getConfig so it doesn't warn when in the test environment.

I think this is good enough until we start writing tests that require these config values.

@catarak
Copy link
Member

catarak commented Jun 15, 2020

works for me!

@catarak catarak merged commit b805754 into processing:develop Jun 15, 2020
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