fix: fix cache error with geo country mock #4821

JWhist · 2022-07-18T13:40:32Z

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes error by validating that no geoCountry value is set before checking the cache.

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

github-actions · 2022-07-18T13:42:24Z

📊 Benchmark results

Comparing with c766d51

Package size: 221 MB

⬇️ 0.00% decrease vs. c766d51

^                  227 MB  227 MB  227 MB  227 MB  227 MB  227 MB  227 MB  227 MB                         
│  221 MB  221 MB   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐   221 MB  221 MB  221 MB 
│   ┌──┐    ┌──┐    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T

Legend

T-30 (44245c0): 221 MB
T-29 (1bd8e21): 221 MB
T-28 (0ae27dd): 221 MB
T-27 (7ce1c98): 221 MB
T-26 (f1f5cfa): 221 MB
T-25 (fe20d82): 221 MB
T-24 (0ca7b74): 221 MB
T-23 (c6928b2): 221 MB
T-22 (b46e971): 221 MB
T-21 (a769168): 221 MB
T-20 (81231ea): 221 MB
T-19 (f08a325): 221 MB
T-18 (e0f1e5d): 221 MB
T-17 (a36bc16): 221 MB
T-16 (a214002): 221 MB
T-15 (9335721): 221 MB
T-14 (e417c6e): 221 MB
T-13 (62452c4): 221 MB
T-12 (36945ec): 221 MB
T-11 (e4a92e3): 221 MB
T-10 (8d7d078): 227 MB
T-9 (7ee7d2a): 227 MB
T-8 (3d285ea): 227 MB
T-7 (ab2c562): 227 MB
T-6 (5d91429): 227 MB
T-5 (8bdd73c): 227 MB
T-4 (e00acbc): 227 MB
T-3 (623fe51): 227 MB
T-2 (65913b4): 221 MB
T-1 (c766d51): 221 MB
T (current commit): 221 MB

eduardoboucas · 2022-07-18T13:47:58Z

src/lib/geo-location.js

@@ -43,7 +43,7 @@ const getGeoLocation = async ({ geoCountry, mode, offline, state }) => {

  // If we have cached geolocation data and the `--geo` option is set to
  // `cache`, let's try to use it.
-  if (cacheObject !== undefined && mode === 'cache') {
+  if (cacheObject !== undefined && mode === 'cache' && !geoCountry) {


This is a bit of an edge case, but if you have something in the cache and it happens to be the same country as the one in geoCountry, it'd probably be better to return the cached content than the mock data.

It might be confusing for someone to be mocking their own country but be consistently returned their own location though, it may lead them to believe the feature is not working. Maybe once the full country names and/or subdivisions are mockable this might be more feasible?

I'm not sure there's ever any situation where you'd rather receive dummy data than an actual location from the country you're trying to emulate. But I'll defer to you.

No, after thinking about it, I agree with you. I think I was originally looking down the road a bit more towards mocking subdivisions/states at which point the caching might be counterproductive. But seeing as that is probably either not going to happen, or a ways off at this point, I think your approach makes sense. I'll abstract out that logic into a function of its own, and it can be modified later if needed.

JWhist · 2022-07-18T14:45:19Z

New caching function allows for the behavior like this in the event that a user mocks their own country:

If a user passes --country alone with no --geo flag, then the mode will default to 'cache' and will use the cache object if the country matches that of the argument, otherwise the usual mock country.
If a user passes --country --geo=mock then the cache will always be bypassed.

eduardoboucas · 2022-07-18T15:14:02Z

src/lib/geo-location.js

@@ -27,6 +27,9 @@ const mockLocation = {
  subdivision: { code: 'CA', name: 'California' },
 }

+const isCacheEligible = (cacheObj, mode, country) =>
+  cacheObj !== undefined && mode === 'cache' && (cacheObj.data.country.code === country || !country)


If you run --country=XX, shouldn't mode become mock? That's what the docs say is happening, and I wonder if reflecting that in the code would make this simpler, because then you wouldn't need to change this check at all?

If we change the mode to mock, I'll still have to change the conditional in order to use the cached location in the case of a match though, right? To something like this?

const cacheObject = state.get(STATE_GEO_PROPERTY) if (geoCountry) mode = 'mock' // If we have cached geolocation data and the `--geo` option is set to // `cache`, let's try to use it. if (cacheObject !== undefined && (mode === 'cache' || cacheObject.data.country.code === geoCountry)) { const age = Date.now() - cacheObject.timestamp

MarcL · 2022-07-19T08:24:07Z

New caching function allows for the behavior like this in the event that a user mocks their own country:

If a user passes --country alone with no --geo flag, then the mode will default to 'cache' and will use the cache object if the country matches that of the argument, otherwise the usual mock country.

If a user passes --country --geo=mock then the cache will always be bypassed.

Does this now need a documentation update @JWhist? The docs suggest that not passing the--geo flag will default to mock and not cache.

src/lib/geo-location.js

eduardoboucas · 2022-07-19T08:49:43Z

Does this now need a documentation update @JWhist? The docs suggest that not passing the--geo flag will default to mock and not cache.

Personally, I think there's no need, because we're still respecting the country you set with --country and I think that's the most important thing. The only extra thing we're doing is saying that if that country matches something that you have in cache, we show the real data as opposed to some random street name etc.

Co-authored-by: Eduardo Bouças <[email protected]>

MarcL

👍🏻 Thanks for the fix.

fix: fix cache error with geo country mock

9139f9f

JWhist requested a review from a team as a code owner July 18, 2022 13:40

JWhist self-assigned this Jul 18, 2022

JWhist requested a review from MarcL July 18, 2022 13:40

github-actions bot added the type: bug code to address defects in shipped code label Jul 18, 2022

eduardoboucas reviewed Jul 18, 2022

View reviewed changes

eduardoboucas previously approved these changes Jul 18, 2022

View reviewed changes

JWhist added 3 commits July 18, 2022 10:41

fix: update caching logic

6109bed

fix: update caching logic

7baf114

fix: update caching logic

25e063f

JWhist dismissed eduardoboucas’s stale review via 25e063f July 18, 2022 14:42

eduardoboucas reviewed Jul 18, 2022

View reviewed changes

JWhist and others added 3 commits July 18, 2022 13:35

fix: revert previous set mode to mock when using country flag

1147117

chore: update contributors field

cdf7883

Merge branch 'main' into geoCountryFix

5cb57e8

eduardoboucas reviewed Jul 19, 2022

View reviewed changes

src/lib/geo-location.js Outdated Show resolved Hide resolved

src/lib/geo-location.js Show resolved Hide resolved

JWhist and others added 7 commits July 19, 2022 09:14

Update src/lib/geo-location.js

5716a5d

Co-authored-by: Eduardo Bouças <[email protected]>

Update src/lib/geo-location.js

f62e208

Co-authored-by: Eduardo Bouças <[email protected]>

Merge branch 'main' into geoCountryFix

e68f83a

chore: run prettier

07b558f

Merge branch 'main' into geoCountryFix

0d4dc8f

Merge branch 'main' into geoCountryFix

45edd03

Merge branch 'main' into geoCountryFix

4ea2bb7

JWhist requested a review from eduardoboucas July 19, 2022 18:39

eduardoboucas approved these changes Jul 20, 2022

View reviewed changes

MarcL approved these changes Jul 20, 2022

View reviewed changes

Merge branch 'main' into geoCountryFix

ef77e14

JWhist enabled auto-merge (squash) July 20, 2022 13:03

JWhist merged commit 419f447 into main Jul 20, 2022

JWhist deleted the geoCountryFix branch July 20, 2022 13:13

token-generator-app bot mentioned this pull request Jul 20, 2022

chore(main): release 10.11.0 #4834

Merged

snyk-bot mentioned this pull request Feb 2, 2023

[Snyk] Security upgrade @netlify/build from 28.4.5 to 29.5.3 MaxMood96/cli-2#143

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix cache error with geo country mock #4821

fix: fix cache error with geo country mock #4821

JWhist commented Jul 18, 2022 •

edited

Loading

github-actions bot commented Jul 18, 2022 •

edited

Loading

eduardoboucas Jul 18, 2022

JWhist Jul 18, 2022

eduardoboucas Jul 18, 2022

JWhist Jul 18, 2022

JWhist commented Jul 18, 2022

eduardoboucas Jul 18, 2022

JWhist Jul 18, 2022

MarcL commented Jul 19, 2022

eduardoboucas commented Jul 19, 2022

MarcL left a comment

fix: fix cache error with geo country mock #4821

fix: fix cache error with geo country mock #4821

Conversation

JWhist commented Jul 18, 2022 • edited Loading

Summary

github-actions bot commented Jul 18, 2022 • edited Loading

📊 Benchmark results

Package size: 221 MB

eduardoboucas Jul 18, 2022

Choose a reason for hiding this comment

JWhist Jul 18, 2022

Choose a reason for hiding this comment

eduardoboucas Jul 18, 2022

Choose a reason for hiding this comment

JWhist Jul 18, 2022

Choose a reason for hiding this comment

JWhist commented Jul 18, 2022

eduardoboucas Jul 18, 2022

Choose a reason for hiding this comment

JWhist Jul 18, 2022

Choose a reason for hiding this comment

MarcL commented Jul 19, 2022

eduardoboucas commented Jul 19, 2022

MarcL left a comment

Choose a reason for hiding this comment

JWhist commented Jul 18, 2022 •

edited

Loading

github-actions bot commented Jul 18, 2022 •

edited

Loading