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

feat(react): refactor util getModuleFederationConfig to avoid to pass function to determinate the remote url #16488

Conversation

fyodorovandrei
Copy link
Contributor

@fyodorovandrei fyodorovandrei commented Apr 23, 2023

Current Behavior

For the utils getModuleFederationConfig you need to pass the parameter determineRemoteUrl.

Expected Behavior

For the util getModuleFederationConfig now no more needs to pass determineRemoteUrl. The util by itself understands which function to call


I open a PR in the Module Federation repository to add supports Module Federation remote containers in Storybook. To consume remote MF apps/components in Storybook I created a new util withModuleFederation to replace the existing NX util. In the new util I'm using getModuleFederationConfig. With this PR we can use util getModuleFederationConfig without passing the parameter determineRemoteUrl

@vercel
Copy link

vercel bot commented Apr 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 27, 2023 10:54am

@fyodorovandrei
Copy link
Contributor Author

@Coly010 You know more about :) If you have time to take a look will be nice. Thx 👍

@jaysoo
Copy link
Member

jaysoo commented Apr 25, 2023

Hey @fyodorovandrei Thanks for the PR. I'll discuss with @Coly010 about this PR, it looks reasonable to me. We do have the Nx 16 release this week, so we will be delayed in reviewing feature PRs.

One behavior change is that getModuleFederationConfig will now be tied to project configuration, which perhaps isn't a big deal since mostly encapsulate it within withModuleFederation util. Alternatively we could provide a default function rather than remove it completely.

}

const host = serveTarget.options?.host ?? 'http://localhost';
const port = serveTarget.options?.port ?? 4201;

Choose a reason for hiding this comment

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

What about using/creating a similar function to the findNextAvailablePort to prevent local port collisions?

…actor-util-get-module-federation-config

# Conflicts:
#	packages/react/src/module-federation/utils.ts
#	packages/react/src/module-federation/with-module-federation-ssr.ts
#	packages/react/src/module-federation/with-module-federation.ts
@fyodorovandrei
Copy link
Contributor Author

fyodorovandrei commented Apr 27, 2023

@Coly010 Why is a lot of duplicated code? Will be a good idea to refactor?

@Coly010
Copy link
Contributor

Coly010 commented Apr 27, 2023

There's no good common place that can be used here and there minor differences between react and angular that need to be accounted for. This is fine as is now

@jaysoo jaysoo merged commit 6dd1385 into nrwl:master Apr 27, 2023
@github-actions
Copy link

github-actions bot commented May 3, 2023

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 3, 2023
@fyodorovandrei fyodorovandrei deleted the features/refactor-util-get-module-federation-config branch June 16, 2023 20:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants