near-wallet icon indicating copy to clipboard operation
near-wallet copied to clipboard

Extract `process.env` into `config.js`

Open MaksymZavershynskyi opened this issue 5 years ago • 8 comments

Wallet is a static so all process.env calls are only relevant during npm run build. Since docker image will not run npn run build upon each start but instead will run it only when it is created env variables will not allow us to control the static content. Instead we should be using config.js similarly to how studio does it and have all parameters there.

MaksymZavershynskyi avatar Jun 13 '19 18:06 MaksymZavershynskyi

you mean similar to config.json in studio, right? Basically have a file which isn't packed using web pack, but loaded each time separately?

however I am not sure we need it for wallet (or other static websites) – an alternative is just always use nginx to rewrite default URLS like https://studio.nearprotocol.com/devnet into whatever should be actual value.

vgrichina avatar Jun 13 '19 19:06 vgrichina

Similar to config.json in studio, yes. But there is no need to make it dynamically loadable, since this is a static content.

however I am not sure we need it for wallet (or other static websites) – an alternative is just always use nginx to rewrite default URLS like https://studio.nearprotocol.com/devnet into whatever should be actual value.

That's exactly the intent. We just have a script that replaces default URLs with other URLs. However, currently it is not possible, because in the wallet there is currently no default value for PUBLIC_URL. (Also, on a different node REACT_APP_ACCOUNT_HELPER_URL seems to default to a docker container id, which is wrong, since this id can be different on everyone's computer, and upon every launch, and it is specific to whatever setup someone who added used). So if we had one place, like config.json (or config.js) with all default variables it would have guaranteed that they are replaceable.

MaksymZavershynskyi avatar Jun 13 '19 20:06 MaksymZavershynskyi

Don't worry about PUBLIC_URL, in practice I think it doesn't get baked in as absolute URL.

vgrichina avatar Jun 14 '19 20:06 vgrichina

What's the current up-to-date way to set these variables (e.g. REACT_APP_NODE_URL)? I'm trying to build a Wallet Docker image and am hitting the same issue @nearmax described

mieubrisse avatar Sep 22 '21 14:09 mieubrisse

Sounds like Kurtosis and @mieubrisse met with Daryl to discuss that it would be non-trivial to use a file for more info. They'll be proceeding by running npm run build before the Docker service starts, which may slow things down a smidge, but is a great solution.

mikedotexe avatar Oct 04 '21 21:10 mikedotexe

Following up: we went with the build-on-every-Docker-container-start method, but it takes quite a while - depending on CPU usage on the machine, sometimes it takes longer than 2m to start. It would be really, really nice if we had a configurable way to start

mieubrisse avatar Oct 18 '21 16:10 mieubrisse

@mieubrisse It feels like 2 minutes build time per run isn't too reasonable, so I've been thinking about this more.

Although I want to keep our production wallet build artifacts deterministic (e.g. keep the env data inside of the bundle), we can add an alternative build configuration type that optimizes for this particular use case to eliminate the startup delay -- we just won't use it for the builds we deploy for production uses.

We recently landed a PR (#2203) that pulls all of our app-relevant process.env references into a single file (src/config.js).

As a result of this change, we may now be able to use an alternative Parcel configuration to externalize that file for the new 'dynamic env' build type, which, if possible, would allow you to replace just that file in your container with your own file (a valid .js module or json file that exports the same named exports), to be loaded when the bundle initializes.

Possibly useful for this purpose: https://github.com/FlorianRappl/parcel-plugin-externals

Unfortunately, I don't have the bandwidth to dig into this any deeper right now, but if this sounds like an acceptable solution, we'd be happy to merge a PR that adds a new command to build with the src/config.js file externalized so that you can inject one into the docker container for runtime configurability.

MaximusHaximus avatar Oct 27 '21 00:10 MaximusHaximus

@MaximusHaximus sorry for the delay in responding here; that sounds incredible! In addition to the 2m time delay, we also discovered with @BenKurrek that the default Docker engine settings (2GB dedicated to Docker) will cause the build of the Wallet inside the container to OOM. So having an externalized config sounds like a huge win - both to speed up module launch so that every module-launched network can come with a Wallet, and so that users dont' have to change their Docker settings!

mieubrisse avatar Nov 05 '21 23:11 mieubrisse