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.

Sign up for GitHub

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

Jump to bottom

frontend: replace vite-plugin-vue2-svg with vite-plugin-svg4vue #5173

Closed
wants to merge 2 commits into from
Closed

frontend: replace vite-plugin-vue2-svg with vite-plugin-svg4vue #5173

wants to merge 2 commits into from

Conversation

BacLuc
Copy link
Contributor

@BacLuc BacLuc commented May 12, 2024

Because vite-plugin-vue2-svg does not support vue3. Only worked when converting vite.config.js to
an esm file by renaming it to .mjs.
vite-plugin-svg4vue is not the most popular package for this, this would be https://www.npmjs.com/package/vite-svg-loader, but that does not support vue2.

Issue: #5121

We can also wait to merge this short before we want to upgrade to vue3.

Also fixes https://github.com/ecamp/ecamp3/security/dependabot/178

@BacLuc BacLuc mentioned this pull request May 12, 2024
Open
9 tasks
@BacLuc BacLuc requested review from usu and carlobeltrame May 12, 2024 19:25
@usu usu added the deploy! Creates a feature branch deployment for this PR label May 25, 2024
@usu usu temporarily deployed to feature-branch May 25, 2024 05:18 — with GitHub Actions Inactive
@github-actions GitHub Actions
Copy link

github-actions bot commented May 25, 2024 •
edited

⛔ Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

usu
usu requested changes May 25, 2024
View reviewed changes
Copy link
Member

@usu usu left a comment

Choose a reason for hiding this comment

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

Seems not to work on deployment

image

@BacLuc
frontend: replace vite-plugin-vue2-svg with vite-plugin-svg4vue
b6e80df 
Because vite-plugin-vue2-svg does not support vue3.
Only worked when converting vite.config.js to
an esm file by renaming it to .mjs.
vite-plugin-svg4vue is not the most popular package
for this, this would be https://www.npmjs.com/package/vite-svg-loader,
but that does not support vue2.

Issue: ecamp#5121

We can also wait to merge this short before we want
to upgrade to vue3.

Also fixes https://github.com/ecamp/ecamp3/security/dependabot/178
@BacLuc
fixup! frontend: replace vite-plugin-vue2-svg with vite-plugin-svg4vue
7040930 
use v-html, because outputting the raw svg code into the element
does not work in production.
@BacLuc
Copy link
Contributor Author

BacLuc commented May 26, 2024

Seems not to work on deployment

image

I hoped we would get away without v-html.
It seems not

fixed

@usu
Copy link
Member

usu commented May 26, 2024

Seems not to work on deployment
image

I hoped we would get away without v-html. It seems not

fixed

Did the ?component method not work? Or why do we need to use ?raw and v-html?

usu
usu reviewed May 26, 2024
View reviewed changes
frontend/vite.config.mjs
@@ -17,7 +17,9 @@ const plugins = [
VuetifyResolver(),
],
}),
createSvgPlugin(),
svg4VuePlugin({
skipSvgo: true
Copy link
Member

Choose a reason for hiding this comment

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

  • What does svgo do? Why do we want to disable it?
  • I think the option key should be svgoConfig, at least according to the docs.

Copy link
Member

Choose a reason for hiding this comment

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

It minimizes the svgs. I'm not sure this is the way to go

manuelmeister
manuelmeister requested changes May 27, 2024
View reviewed changes
Copy link
Member

@manuelmeister manuelmeister left a comment

Choose a reason for hiding this comment

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

Why do we want to use this plugin when we can just replace the plugin once we move to vue3? The vue3 one looks like a drop in replacement for our current one

@BacLuc
Copy link
Contributor Author

BacLuc commented May 28, 2024

Why do we want to use this plugin when we can just replace the plugin once we move to vue3? The vue3 one looks like a drop in replacement for our current one

I am not a fan of big rewrites.
If you change a lot of things, you have to test a lot of things.
Then you forget to test something, and all devs have to work with a broken main branch.
My goal ist to keep the changes for the upgrade to vue3 as small as possible, that we have a working vue3 setup with as little effort and complications as possible.
And then we can work separately to improve the different parts.

@manuelmeister
Copy link
Member

I vote for vite-svg-loader. It is already used in the print package and is a drop in replacement for vite-plugin-vue2-svg

@BacLuc
Copy link
Contributor Author

BacLuc commented Jun 15, 2024

I vote for vite-svg-loader. It is already used in the print package and is a drop in replacement for vite-plugin-vue2-svg

But not compatible with vue2

@manuelmeister manuelmeister added the Meeting Discuss Am nächsten Core-Meeting besprechen label Jun 15, 2024
@manuelmeister
Copy link
Member

Core Meeting Decision

We want to try, if the vite-svg-loader is a drop in solution, otherwise we will try this PR again.

@manuelmeister manuelmeister removed Meeting Discuss Am nächsten Core-Meeting besprechen deploy! Creates a feature branch deployment for this PR labels Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@usu usu usu requested changes

@manuelmeister manuelmeister manuelmeister requested changes

@carlobeltrame carlobeltrame Awaiting requested review from carlobeltrame

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@BacLuc @usu @manuelmeister