Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

Remove use of DefinePlugin, switch to VUE_APP env vars #332

Merged
merged 5 commits into from
Mar 3, 2020

Conversation

jjnesbitt
Copy link
Member

This uses a more standard way of injecting env vars, and removes most of the complexity from vue.config.js. I've also created a .env.default file, as a template that describes the new env vars.

Let me know what you guys think.

client/package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@waxlamp waxlamp left a comment

Choose a reason for hiding this comment

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

This is awesome. I have one idea for this, and I would like to know what you think: should we have a new module, environment.ts or something, which could just export all of the VUE_APP_* variables we make use of? It would be a nice way to express the "interface" between the client app and the environment, and it would also make clear what the default values are that the app is using, all in one place.

CI is currently choking on the client testing; I'll take a look at that.

Copy link
Collaborator

@waxlamp waxlamp left a comment

Choose a reason for hiding this comment

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

Looks awesome. Just the one small change and this is good to merge (as soon as we figure out the testing issue).

client/src/components/AboutDialog.vue Show resolved Hide resolved
@JackWilb
Copy link
Member

JackWilb commented Mar 3, 2020

Looks like the client comes up during testing and on my local. However, running yarn test:client:up on my local fails because of one command, curl -s -I --max-time 0.5 http://localhost:8080/api/workspaces 404s.

waxlamp
waxlamp previously approved these changes Mar 3, 2020
client/src/components/AboutDialog.vue Show resolved Hide resolved
@waxlamp
Copy link
Collaborator

waxlamp commented Mar 3, 2020

Looks like the client comes up during testing and on my local. However, running yarn test:client:up on my local fails because of one command, curl -s -I --max-time 0.5 http://localhost:8080/api/workspaces 404s.

Ah I figured it out: the point of this PR is that it detaches the server from the client altogether. So it's no longer viable to use localhost:8080/api to probe whether the client build has succeeded; however, we can do the same thing by just curl'ing localhost:8080 instead. See 8e3d7a5.

Copy link
Member

@JackWilb JackWilb left a comment

Choose a reason for hiding this comment

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

Looks good!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants