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

GSLUX-664: Proxyurl as a setting from v3 #95

Merged
merged 5 commits into from
Apr 25, 2024
Merged

Conversation

AlitaBernachot
Copy link
Contributor

@AlitaBernachot AlitaBernachot commented Apr 18, 2024

JIRA issue

https://jira.camptocamp.com/browse/GSLUX-664

Description

  • Created a new helper ProxyUrlHelper
  • Allow initialization of ProxyUrlHelper through ProxyUrlHelper.init_v3() with some params (⚠️ To be used only in v3, this function should be removed when v4 is fully ready)
  • Default params are declared in new .env: VITE_USE_PROXYURL, VITE_PROXYURL_WMS, VITE_PROXYURL_REMOTE

Don't know if adding a .env is the best idea, if there are some app settings to be configured, a settings.json (loaded with import settings.json) could be better. But this can be improve in another PR.

This PR is linked to Geoportail-Luxembourg/geoportailv3#3133

Copy link
Contributor

GitHub Pages links: * Luxembourg-geoportail: https://geoportail-luxembourg.github.io/luxembourg-geoportail/GSLUX-664-proxywmsurl/

Copy link
Contributor

@mki-c2c mki-c2c left a comment

Choose a reason for hiding this comment

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

works well.

I just wonder if we shouldn't use a store instead of a global object for more genericity.

and there seems to be a typo in the test launch command

@@ -0,0 +1,58 @@
const forceUseProxy = import.meta.env.VITE_USE_PROXYURL === 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we use a store for the proxy properties ?
Changes need not be monitored, but it will be more consistent with other "global variables" saved as a state in a store

Copy link
Contributor Author

@AlitaBernachot AlitaBernachot Apr 25, 2024

Choose a reason for hiding this comment

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

At first, I put the variables in a store. But I wasn't convinced after a few tests because:

  • proxyurls vars never change, they depend only on the env the app is launched on
  • if it is comming from a store, it means: the helper should only be used inside a vue component OR the helper should be used once the store is fully initialized (with the right values). It can be done using watch() in v3, but sometimes the layers are created before the init of the proxyhelper, I have no control on that (too complicated for now).

So I made it simple, because these urls never change and they depend on the environment, I put them as environment variables.

I have made some changes on the Proxyhelper to make it clear that initialization is needed only because we need to retrieve environment vars from v3, otherwise initialization in v4 is pointless.

Also, I remember we have discussed about a potential configuration store last year, maybe later this can be moved into this future store when we have time.

package.json Outdated Show resolved Hide resolved
@AlitaBernachot AlitaBernachot merged commit 3c5ae7b into main Apr 25, 2024
2 checks passed
@AlitaBernachot AlitaBernachot deleted the GSLUX-664-proxywmsurl branch April 25, 2024 12:55
@mki-c2c mki-c2c mentioned this pull request Jun 13, 2024
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