Skip to content

Cross platform vite_import plugin #5513

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
Open
edisile wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Cross platform vite_import plugin #5513

edisile wants to merge 8 commits into from

Conversation

@edisile
Copy link
Contributor

@edisile edisile commented Dec 4, 2025 •
edited
Loading

Done

  • replaced grep command with ripgrep (pulled using @lvce-editor/ripgrep) for cross-platform support
  • improved vite_import detection plugin
  • moved the plugin to a separate file
    • at some point it could also be moved to a separate package

Testing

  • This PR has tests
  • No testing required (explain why): if tests pass it's good

@webteam-app
Copy link

webteam-app commented Dec 4, 2025

Demo

Jenkins

demos.haus

alvaromateo
alvaromateo reviewed Dec 5, 2025
View reviewed changes
Copy link
Contributor

@alvaromateo alvaromateo left a comment

Choose a reason for hiding this comment

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

Just a concern regarding support for Windows and new line characters.

viteDetectImportPlugin.js Outdated

// the string has multiple lines, some of which might be blank, so split
// on newlines and filter out the empty strings
const imports = viteImports.split("\n").filter(Boolean);
Copy link
Contributor

@alvaromateo alvaromateo Dec 5, 2025

Choose a reason for hiding this comment

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

In the captureString you consider support for Windows platform (brave 😂).
I believe Windows uses a different new line characters. Won't that be a problem here (and later in the .join("\n"))?

Copy link
Contributor Author

@edisile edisile Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, splitting this way leaves a bad "\r" in the filename... Vite doesn't seem to mind but I'll add a check for this edge case. No need for "\r\n" in the .join call because that only gets printed in the console

steverydz
steverydz approved these changes Dec 5, 2025
View reviewed changes
Copy link
Contributor

@steverydz steverydz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@edisile
Copy link
Contributor Author

edisile commented Dec 9, 2025

There are some connection time out issues when downloading ripgrep from github that can lead to builds failing. Adding "don't merge" label until I find some time to investigate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steverydz steverydz steverydz approved these changes

@alvaromateo alvaromateo alvaromateo approved these changes

Assignees

No one assigned

Labels

Don't merge Review: Code +1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@edisile @webteam-app @steverydz @alvaromateo