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

Enable the client to reflect the PC URL used by the server. #1457

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

jvwong
Copy link
Member

@jvwong jvwong commented Jun 7, 2024

Retrieve the PC_URL from the server endpoint, cache in client.

Refs: #1456

@jvwong jvwong requested a review from IgorRodchenkov June 7, 2024 17:19
@jvwong
Copy link
Member Author

jvwong commented Jun 7, 2024

This isn't a general solution, e.g. FACTOID_URL is a config variable, but works for our purposes. Our React version is pretty old (docs aren't even available) and there are some new features to handle these sorts of things....

Copy link
Member

@IgorRodchenkov IgorRodchenkov 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, thank you!

PS (somewhat off-topic:

Before v4.0.0 release of this app for PC v14 backend (new app-ui will not completely work with the older versions/data anymore due to some env/config changes and using bioregistry.io instead of identifiers.org, etc.) -

  • can we make sure we have correct Dockerfile for prod (I bet we do not need imges for dev use anyway...). Is this line there still correct: RUN NODE_ENV=development npm ci (I guess NODE_ENV is not needed or must be production, as we do not want any dev modules/dependencies in docker images, and it should be npm install instead of ci.. but I am not sure)?
  • check/polish Readme as well...
  • do we really want that "Syblars" and cli.js be part of this web app or those could be separated into another Git repo perhaps?

@@ -1,5 +1,5 @@
const _ = require('lodash');
const logger = require('../../logger');
// const logger = require('../../logger');
Copy link
Member

Choose a reason for hiding this comment

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

If we do not need logger in this file, then let's remove this line instead of commentig it out...

@@ -4,6 +4,11 @@ const pc = require('../external-services/pathway-commons');

const router = express.Router();

const { PC_URL } = require('../../config');

router.get('/baseURL', function (req, res) {
Copy link
Member

Choose a reason for hiding this comment

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

I like this - so it will use the PC_URL env property set at runtime for both server and client side now.

@IgorRodchenkov
Copy link
Member

IgorRodchenkov commented Jun 8, 2024

@jvwong , unlike FACTOID_URL, which is only used by the server-side/backend (client calls webapp's /api/biofactoid proxy instead of calling biofactoid api directly), PC_URL was used both from backend and client side. There was just one use case - downloading data in varios formats directly from PC ws as saving as files - where PC_URL was used from the client.
This PR here seems to remove the pre-built/default PC_URL value from the app-ui bundle.js and so the PC_URL environment property set at runtime now applies not only to the backend but also to those client's "download" links!

I think there is something wrong about const envVars = ['NODE_ENV', 'PC_URL', 'FACTOID_URL']; in the webpack.config.js - likely we do not need the latter two build-time options anymore... these were either never actually used or did not pick up the runtime env value, like in PC_URL case... We should be able to set/update at run time (e.g. for a container) any of those ENV properties listed in config.js. Also, I think we do not need any of those DB_* properties...

@IgorRodchenkov IgorRodchenkov merged commit 8d6dd9c into development Jun 8, 2024
@jvwong jvwong deleted the iss1270_dynamic-client-envs branch June 11, 2024 20:09
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