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

Add support for using the vite dev server on remotes #551

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

Conversation

jlillywhite
Copy link

Description

This adds support for using the vite dev server for remotes. This addresses issue 204

Additional context

While this appears to work with the examples and the project where I'm consuming it, I'm not sure that it's very efficient. If there are ways to be more efficient with importShared and its associated module cache I would appreciate the input.

I also didn't find a way to support version checking importShared without copying the satisfy code.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Code of Conduct and follow the Commit Convention guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@ruleeeer
Copy link
Collaborator

ruleeeer commented Dec 25, 2023

Great, thank you for your contribution.
I previously attempted to add the remoteEntry file in dev mode, but failed due to Vite's bundless characteristic.

@ruleeeer
Copy link
Collaborator

When I have some free time, I will start the code review.

@hodfords-viet-fe
Copy link

@ruleeeer
Please help review this PR, we really need this feature for development.
Thanks

@AntonyF-Andreani
Copy link

Hi @ruleeeer! Thanks for the time for mantain this awesome plugin!

Maybe I can help you adding some examples or test to evaluate if this PR is possible? (to reduce the time or the responsability of the review)

I would be happy to help!!

@ruleeeer
Copy link
Collaborator

@jlillywhite @jlillywhite-mc
Thanks for your great work, I tried to test with vue3-demo-esm, but I found that it fails, and since there is no modification to the documentation for this PR, I'm not sure if I'm missing something important in the config

@ruleeeer
Copy link
Collaborator

ruleeeer commented Jan 20, 2024

I added a line to packages/examples/vue3-demo-esm/package.json that I can use to test the effect of dev remotes

dev:remotes": "pnpm --parallel --filter \"./home\" --filter \"./css-modules\" --filter \"./common-lib\" --filter \"./dynamic-remote\"  dev"

Or go to packages/examples/vue3-demo-esm/css-modules and execute pnpm dev to see the error as well
image

@ruleeeer
Copy link
Collaborator

ruleeeer commented Jan 20, 2024

Hi @ruleeeer! Thanks for the time for mantain this awesome plugin!

Maybe I can help you adding some examples or test to evaluate if this PR is possible? (to reduce the time or the responsability of the review)

I would be happy to help!!

Thanks for providing help !!
I would suggest waiting until this PR fix is complete before adding test cases.

1): Test cases are needed, the easiest way to do this is to add a start dev:remotes to the test project like I said

#551 (comment)

2): Then change the test dev's script after.
Change it so that both hosts and remotes are started with dev.

https://github.com/originjs/vite-plugin-federation/blob/main/packages/examples/vitestSetup-dev.ts#L83-L84
image

@jlillywhite
Copy link
Author

Hey, thanks for taking the time to look at this. I will update it with tests and fixes as soon as I can.

@kunal-gc
Copy link

kunal-gc commented Feb 7, 2024

Any idea when this will get merged? I also need this.

@PicoI2
Copy link
Contributor

PicoI2 commented Feb 12, 2024

I tried to use this PR in my project, it does not work for me. I have warnings like
'You are importing createRoot from "react-dom" which is not supported. You should instead import it from "react-dom/client".' and others telling me that some modules are already loaded.
I will try to understand why.

@jlillywhite
Copy link
Author

I'm still seeing a few issues with sharing images and manually setting package paths. I will continue working on this.

@PicoI2
Copy link
Contributor

PicoI2 commented Feb 19, 2024

I have noticed that if you use vite-plugin-react, both host and remote have to be in dev mode.
If not, you will have this error message "@vitejs/plugin-react can't detect preamble".

@PicoI2
Copy link
Contributor

PicoI2 commented Feb 20, 2024

I tried to use this PR in my project, it does not work for me. I have warnings like 'You are importing createRoot from "react-dom" which is not supported. You should instead import it from "react-dom/client".' and others telling me that some modules are already loaded. I will try to understand why.

It was because I had enable profiling on the host and not on the remote :
resolve: { alias: { "react-dom/client": "react-dom/profiling" } }
Without it it is working, I will try continue to test it but it's good news !

@jlillywhite
Copy link
Author

I have noticed that if you use vite-plugin-react, both host and remote have to be in dev mode. If not, you will have this error message "@vitejs/plugin-react can't detect preamble".

I ran into this same issue when I was working on this. I'm still working on a clean way to resolve it.

@MrDeerly
Copy link

bump

@MrDeerly
Copy link

MrDeerly commented Apr 9, 2024

@ruleeeer @PicoI2 @jlillywhite How can we help you guys to unblock this and get it merged?

@jlillywhite
Copy link
Author

@MrDeerly, I'm primarily a React developer, so it was pretty easy for me to get this working in dev mode for React, but I'm not familiar some of the other UI frameworks, so I'm having trouble tracking down the issues in other frameworks. If you have the expertise to do that, I would love any help I could get.

@PicoI2
Copy link
Contributor

PicoI2 commented Apr 10, 2024

I think it could be merged as using dev mode is not working at all without this, it's not like it will break anything.
I tried this PR in a large project and its working great.

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.

None yet

8 participants