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

feat: allow data dir to be moved #173

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

stezu
Copy link
Contributor

@stezu stezu commented Jun 4, 2024

I'm running into the same issue as #159 due to using a build process which bundles all of my code and dependencies for AWS Lambda functions. That build process means that the JSON files within the data dir of this package are bundled automatically because they use require, but the .dat files are not since they're loaded at runtime using fs.

I can copy the data dir into my build folder using copy-webpack-plugin (or similar for rollup/esbuild/vite processes) so it is available to the runtime, but after bundling, the location of the geo-tz JS would be in an unexpected place so the ../data location it's looking for is most likely going to be incorrect. In my case, this package ended up in the root folder of my Lambda bundle, so ../data was outside of the file system I had access to modify. For AWS Lambda specifically, we can also make use of layers to store static dependencies, but the path will again most likely not be available to the ../data path this package expects.

To remedy this issue, this PR adds support for a GEO_TZ_DATA_PATH environment variable that allows the data directory to be moved or renamed as needed for each use case (his is effectively the same solution as #140, but updated to support the different data sets added in #162/#163).

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.41%. Comparing base (46be41e) to head (823e924).
Report is 7 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   83.70%   89.41%   +5.70%     
==========================================
  Files           7        7              
  Lines         356      359       +3     
  Branches       60       63       +3     
==========================================
+ Hits          298      321      +23     
  Misses         36       36              
+ Partials       22        2      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@evansiroky
Copy link
Owner

Ok cool, since I'm getting 2 different requests for the same solution, it seems like it ought to be merged. One request though, if you're able, can you please:

  1. Remove the change to the package-lock.json file
  2. Add a short entry into the README explaining the availability of this environment variable?

@stezu
Copy link
Contributor Author

stezu commented Sep 10, 2024

@evansiroky thanks for taking a look! I just made those changes.

@evansiroky evansiroky merged commit 3a4072e into evansiroky:master Sep 10, 2024
3 checks passed
@evansiroky
Copy link
Owner

🎉 This PR is included in version 8.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants