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

plugins: Migrate plugins to vite #2001

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

plugins: Migrate plugins to vite #2001

wants to merge 3 commits into from

Conversation

sniok
Copy link
Contributor

@sniok sniok commented May 29, 2024

This PR continues vite migration. First part is in PR here #1981

Fixes this #1282

Testing done

  • tested with examples, and plugins from plugin repo

@sniok sniok changed the base branch from main to vite May 29, 2024 13:26
@sniok sniok force-pushed the vite branch 3 times, most recently from b852dc6 to c553a32 Compare May 31, 2024 11:18
@sniok sniok force-pushed the vite-plugins branch 5 times, most recently from e233d67 to 643bd34 Compare June 11, 2024 11:23
@sniok sniok marked this pull request as ready for review June 11, 2024 11:39
Base automatically changed from vite to main June 14, 2024 13:43
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

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

Can you please rebase against main?

(Also I'm not sure if that snyk check failing is still relevant? I thought you had made a change for that issue already?)

Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

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

Noticed a few things.

Seems the package exclude change needs to be added in here as well? (for the snyk license check)

@@ -1,5 +0,0 @@
// @ts-nocheck
import { initTests } from '@kinvolk/headlamp-plugin/config/storyshots/storyshots-test';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure this can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since it's a template for new plugins so I don't see a risk there. If storyshots plugin is deprecated I think it's fair for us to not support it. Keeping storyshots tests for plugins is not trivial, we would effectively need to maintain a copy of 'storyshots-tests' plugin that works with vite

plugins/headlamp-plugin/package.json Outdated Show resolved Hide resolved
frontend/package-lock.json Outdated Show resolved Hide resolved
@illume illume marked this pull request as draft June 19, 2024 11:27
@illume illume mentioned this pull request Jun 26, 2024
5 tasks
@illume
Copy link
Collaborator

illume commented Jun 26, 2024

The issue for this is here: #1282 (should go in the PR description)

@sniok sniok force-pushed the vite-plugins branch 2 times, most recently from 3127480 to 01c5d0f Compare June 28, 2024 11:40
@illume illume closed this Jun 28, 2024
@illume illume reopened this Jun 28, 2024
@sniok sniok force-pushed the vite-plugins branch 2 times, most recently from a2bdcef to 1bcc4cb Compare July 2, 2024 14:11
@sniok sniok marked this pull request as ready for review July 2, 2024 14:59
@illume
Copy link
Collaborator

illume commented Jul 8, 2024

Does this allow plugins to access environment variables at build time too? Here's the issue for that:

(but it shouldn't block this PR being merged if it doesn't support it yet)

@sniok
Copy link
Contributor Author

sniok commented Jul 9, 2024

It does not provide process now and I'm not sure if it should. Plugin authors can use import.meta.env for env variables, and if they really need process they can install polyfill in the plugin

@sniok
Copy link
Contributor Author

sniok commented Jul 11, 2024

Rebased and fixed merge conflicts

@sniok sniok requested a review from illume July 11, 2024 10:00
@sniok sniok force-pushed the vite-plugins branch 2 times, most recently from ba88c17 to 41cd299 Compare July 18, 2024 13:57
@sniok
Copy link
Contributor Author

sniok commented Jul 18, 2024

Added interop: auto to the vite config that builds plugins

This allows plugins use default imports from pluginLib the same way webpack plugins do. It checks for the __esModule field and will get the default export.

Tested the Kompose plugin (built with vite) with main (vite) and with 0.24.1 (webpack)

With this change all combinations of webpack frontend, vite frontend, webpack plugin, vite plugin should work together

@sniok
Copy link
Contributor Author

sniok commented Jul 19, 2024

Did some additional testing
Collected some public plugins + example plugins (17 total) compiled all of them with vite and webpack separately.
Ran them on latest main and then 0.24.1 and I didn't see any obvious problem or crashes. Seems like we won't have to mark them as incompatible

@illume illume added this to the v0.26.0 milestone Jul 24, 2024
@sniok sniok force-pushed the vite-plugins branch 4 times, most recently from 4750b3e to b3f188a Compare September 12, 2024 12:41
@sniok sniok marked this pull request as draft September 18, 2024 08:21
@sniok sniok force-pushed the vite-plugins branch 6 times, most recently from 0977a49 to b20d38f Compare September 18, 2024 12:23
atob function is enough and simpler

Signed-off-by: Oleksandr Dubenko <[email protected]>
Signed-off-by: Oleksandr Dubenko <[email protected]>
@sniok sniok marked this pull request as ready for review September 18, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants