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

Allow hard-coded "sheet" URL to be passed at build time #294

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

avengerpenguin
Copy link

This PR changes a few small things so I'd happily take some feedback and chat about whether they are all a net good for the project. I've done them for myself, but perhaps it improves things for others too.

In summary:

  1. The webpack build allows env vars (e.g. from Docker build args) to set the sheet ID (URL) and name at build time

  2. The code itself is refactored a little to allow dependency-injection of the sheet URL

  3. The Dockerfile does the webpack build etc. at build time

  4. The Docker build is now multistage which makes it simpler

Rationale

I liked the idea of being able to run the project as it is while also being able to run a build, even from my own Dockerfile elsewhere, that spits out an HTML file that is pre-baked with the radar data. This would allow deploying it as part of a wider static site, e.g. on Github pages.

It seems a reasonable code architecture to hoist out mentions of window to the top-level file and inject that info in such that all JS files from factory.js are abstracted away from the fact that info came from a query string.

Docker Build Changes

The Docker container running the bulk of the build as a CMD on start seemed odd when -- as far as I can see -- the build output is entirely a function of source files alone.

Maybe there's more flexibility intended around being able to swap out env vars without having to rebuild the container?

I technically do not need the changes for the sheet URL injection, but I'd have to add something to pass the build args into the environment at container runtime.

Small Tweaks

Some stuff I noticed along the way:

  1. There's two webpack configs but they're very close so I hoisted up the entry part to the common file. There may be a reason it is the way it is.

  2. I kept finding it odd to have lots of references to "sheet" and "Google Sheet Input" throughout when it's clearly been extended to allow generic JSON and CSV outside of Google sheets. I suspect this is historic but I changed the name of the top export from factory in the hope this is a positive change in the right direction. I can revert if needed though.

  3. For JSON and CSV input, the h1 title just uses the file name. Since we can now inject I've allowed a SHEET_NAME to be set at build time to override that behaviour.

Tests

I've checked no tests fail, but not added any since the JS changes are meant to be a refactor but perhaps this change warrants an e2e test to verify the new functionality made available.

This PR changes a few small things so I'd happily take some feedback
and chat about whether they are all a net good for the project. I've
done them for myself, but perhaps it improves things for others too.

In summary:

1. The webpack build allows env vars (e.g. from Docker build args) to
set the sheet ID (URL) and name at build time

2. The code itself is refactored a little to allow
dependency-injection of the sheet URL

3. The Dockerfile does the webpack build etc. at build time

4. The Docker build is now multistage which makes it simpler

Rationale
---------

I liked the idea of being able to run the project as it is while also
being able to run a build, even from my own Dockerfile elsewhere, that
spits out an HTML file that is pre-baked with the radar data. This
would allow deploying it as part of a wider static site, e.g. on
Github pages.

It seems a reasonable code architecture to hoist out mentions of
`window` to the top-level file and inject that info in such that all
JS files from `factory.js` are abstracted away from the fact that info
came from a query string.

Docker Build Changes
--------------------

The Docker container running the bulk of the build as a `CMD` on start
seemed odd when -- as far as I can see -- the build output is entirely
a function of source files alone.

Maybe there's more flexibility intended around being able to swap out
env vars without having to rebuild the container?

I technically do not need the changes for the sheet URL injection, but
I'd have to add something to pass the build args into the environment
at container runtime.

Small Tweaks
------------

Some stuff I noticed along the way:

1. There's two webpack configs but they're _very_ close so I hoisted
up the `entry` part to the common file. There may be a reason it is
the way it is.

2. I kept finding it odd to have lots of references to "sheet" and
"Google Sheet Input" throughout when it's clearly been extended to
allow generic JSON and CSV outside of Google sheets. I suspect this is
historic but I changed the name of the top export from factory in the
hope this is a positive change in the right direction. I can revert if
needed though.

3. For JSON and CSV input, the `h1` title just uses the file name.
Since we can now inject I've allowed a SHEET_NAME to be set at build
time to override that behaviour.

Tests
-----

I've checked no tests fail, but not added any since the JS changes are
_meant_ to be a refactor but perhaps this change warrants an e2e test
to verify the new functionality made available.
@devansh-sharma-tw
Copy link
Contributor

Hi @avengerpenguin ,

Thanks for raising this PR!
Sorry for the delay in responding back. We are currently in discussion with the team regarding this PR and will update here shortly.

Thanks!

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