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 custom 'publicPath' config #354

Merged
merged 3 commits into from
Nov 9, 2023

Conversation

chrismayer
Copy link
Collaborator

This introduces an ENV VAR WGU_PUBLIC_PATH in order to change the publicPath Vue CLI configuration, which is used to create the production build, without modifying the Wegue code base.

Also changes the default for publicPath from '/' to './', since the relative notation './' works for most cases, e.g. if the app is deployed at the root of a domain and for a deployment under a certain path. For any other special case use the WGU_PUBLIC_PATH as stated above.

See https://cli.vuejs.org/config/#publicpath

This introduces an ENV VAR 'WGU_PUBLIC_PATH' in order to change the
'publicPath' Vue configuration, which is used to create the production
build, without modifying the Wegue code base.
Also changes the default for 'publicPath' from '/' to './'.
Copy link
Collaborator

@JakobMiksch JakobMiksch left a comment

Choose a reason for hiding this comment

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

I am not sure how to test that.
I ran WGU_PUBLIC_PATH=foo npm run build , but it is not clear to me on which URL this is supposed to run. I ran a webserver both inside the build directory and one level higher. In both cases I could not open the webmap.

@sronveaux
Copy link
Collaborator

Hi @JakobMiksch,

The idea to test this here is to define the ENV_VAR inside a file called .env.production as explained in the Vue-CLI official documentation.

By defining it in this file, you should be able to test this PR.

Regards,
Sébastien

Copy link
Collaborator

@sronveaux sronveaux left a comment

Choose a reason for hiding this comment

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

Hi @chrismayer,

I made some tests here, defining the new environment variable and NOT defining it and managed to run a WebServer to make a complete app to run thoroughly in all cases. So yes, I think this works as expected.

However, for this new environment variable to be fully understandable, shouldn't it be documented somewhere ? This is not easy as there are no documentation section concerning potential environment variables and all what is related to configuration is linked to the app-conf.json files, so I don't really see where it could fit nicely for now... perhaps, like what is done for application contexts, a little note inside the workshop in the build for production part could do the trick ?

@chrismayer
Copy link
Collaborator Author

Thanks for your feedback and testings @sronveaux and @JakobMiksch. I totally agree that a little documentation of the ENV VAR would be really helpful. So I added it to the README in the section Production Build and to the workshop in the section Build for production as suggested by @sronveaux. If necessary we could extend and improve those documentations.

Thnaks again. I am going to merge now...

@chrismayer chrismayer merged commit e5f69df into wegue-oss:master Nov 9, 2023
1 check passed
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.

3 participants