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: Move Repo to NX #154

Merged
merged 62 commits into from
Sep 23, 2022
Merged

Conversation

pavandv
Copy link
Member

@pavandv pavandv commented Sep 11, 2022

This PR addresses #126

  • Configure NX Workspace
  • Configure nextjs-mf package to Build & Publish
    • Use SWC to build the package
    • Migrate the package code to Typescript.
  • Configure node package in NX for Build & Publish
    • Use TSC to build the package
    • Migrate the package code to TS.
  • Migrate Demos to applications using @nrwl/next package
    • Migrate Home app
    • Migrate Shope app
    • Migrate Checkout app

fixes #199, fixes #205, fixes #144, closes #126

@pavandv pavandv linked an issue Sep 11, 2022 that may be closed by this pull request
@ScriptedAlchemy
Copy link
Member

Typescript migration can be a followup PR.
LEts get the plugin + demos into NX first so development is easier on all branches

@nodkz
Copy link
Collaborator

nodkz commented Sep 14, 2022

Firstly need to merge #103 before applying changes to demo apps with nrwl/next.

PR #103 almost ready. I need several hours to finalize it (comments in code, get rid of console log).

They're a lot of changes. And it might be a crazy challenge in conflict resolving.

@pavandv
Copy link
Member Author

pavandv commented Sep 14, 2022

I'm not migrating to TS yet.. We are going with JS first, so it should be fairly easy to merge PR #103 with a rebase.

@nodkz

This comment was marked as outdated.

@pavandv
Copy link
Member Author

pavandv commented Sep 14, 2022

Hmm I see, Perhaps I'll wait for these PR's to be Merged.

@ScriptedAlchemy
Copy link
Member

If these were git move operations, then IDE might be able to pick up there was movement and not cause a major issue

@pavandv
Copy link
Member Author

pavandv commented Sep 14, 2022

I don't think we can perform a git move operation for demo folder, since we will be executing nx generate command to create next app inside apps folder.

@ScriptedAlchemy
Copy link
Member

Ahhh gotcha. Damn

Okay, well I guess that we will need to merge all our work (SSR too) them let you cut us over to nx and then resume working on it :)

I'll follow up tomorrow. Just need to add default option to disable SSR so we can merge it without it impacting anyone

@pavandv
Copy link
Member Author

pavandv commented Sep 15, 2022

@ScriptedAlchemy I'm thinking instead of a separate repo for mf/node we can use the mf/nextjs-mf repo after NX is integrated..

Since nextjs-mf is heavily dependent on the mf-node package, they both living in the same repo makes our work seamless. Otherwise we have to deploy the changes from mf-node first and then consume it on the nextjs-mf later..

I know there are other methods like npm/yarn link to achieve the dev effort.. But I personally think having the mf-node package in the same NX repo works well.

@pavandv pavandv requested a review from nodkz September 20, 2022 13:48
@nodkz

This comment was marked as duplicate.

@nodkz

This comment was marked as duplicate.

@@ -1,51 +1,64 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pavandv what about "dev": "rm -rf lib && concurrently \"yarn sync-files --watch\" \"yarn compile --watch\" \"yarn demo\"", script?

How to run repo in dev mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Figured a way to resolve the issue in another way.. Working on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

To serve applications, one has to run npm/yarn start.

To build packages, one has to run npm run build or yarn build

@nodkz
Copy link
Collaborator

nodkz commented Sep 20, 2022

@pavandv could we please discuss changes in proper review comment threads? After reaching an agreement we will close them.

I hide several of our comments from the main thread because very fast it will become messy.

@@ -18,3 +18,17 @@ module.exports = {
'/wrong-entry',
],
};

export const getRemotes = (isServer: boolean) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we please completely remove this package?

getRemotes is never used.

mfRouters can be placed directly in apps/ folder OR its content can be copy pasted in every _app.tsx file. I think 2nd option is better.

mfRemote is designed for generating in runtime (it can be dynamic and contains different URLs for remotes), and putting it in the package completely broke and hide this idea from newcomers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed 👍

@ScriptedAlchemy
Copy link
Member

Okay this is great. We will start working on this branch and patch over the lastes from main into NX

@pavandv
Copy link
Member Author

pavandv commented Sep 22, 2022

Okay this is great. We will start working on this branch and patch over the lastes from main into NX

@ScriptedAlchemy Patched all the changes from Main branch.

@pavandv pavandv requested a review from nodkz September 22, 2022 12:51
@ScriptedAlchemy
Copy link
Member

how does this thing build? i see rollup is no more :O

@pavandv
Copy link
Member Author

pavandv commented Sep 23, 2022

It uses Typescript Compiler (TSC) to build.

Basically it transpiles all the files to common-js instead of bundling them and I'm controlling what parts of library users can access through barell exports.

We can also use SWC to build the library if we want to.

@ScriptedAlchemy ScriptedAlchemy merged commit d2a4dfa into main Sep 23, 2022
@ScriptedAlchemy ScriptedAlchemy deleted the 126-move-repo-to-nx-based-monorepo branch September 23, 2022 21:12
Copy link
Contributor

🎉 This PR is included in version 1.0.0-canary.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants