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

Migrate to Nx monorepo #371

Closed
wants to merge 23 commits into from
Closed

Migrate to Nx monorepo #371

wants to merge 23 commits into from

Conversation

jeremylvln
Copy link
Contributor

@jeremylvln jeremylvln commented Jan 21, 2022

This PR converts this Lerna monorepo to a Nx one. Closes #143.
Note: this PR is based on #342.

I've done no functional changes except lint fixes. I work with Nx every day, so I guess this implementation is somewhat ideal. Sorry for the large diff, a lot of files has been moved.

I've modified the CI accordingly, do not worry about the MansaGroup/nrwl-nx-action action used inside, I've created it :)

Tests are welcomed!

@jeremylvln jeremylvln marked this pull request as draft January 21, 2022 14:46
@jeremylvln jeremylvln changed the title Test Migrate to Nx monorepo Jan 21, 2022
@underfisk
Copy link
Contributor

@IamBlueSlime Thanks a bunch for doing this, our priority is getting v8 PR first and then this should be our next step

@jeremylvln
Copy link
Contributor Author

@IamBlueSlime Thanks a bunch for doing this, our priority is getting v8 PR first and then this should be our next step

That's why this PR is rebased on the other one :D

@WonderPanda
Copy link
Collaborator

Wondering if it would be easier to just do a fresh migration over to NX now that the upgrade to Nest 8 is done. There's a huge number of duplicated commits and merge commits here that we don't really need.

@jeremylvln
Copy link
Contributor Author

I will rebase the PR correctly asap

@jeremylvln jeremylvln marked this pull request as ready for review January 24, 2022 09:45
@jeremylvln
Copy link
Contributor Author

jeremylvln commented Jan 24, 2022

Hi @WonderPanda, @underfisk. I've rebased the PR, I hope that I've not removed any commit during the process. As this PR moves a lot of file, pay attention to the modification of these file if I've missed something.

Btw, I've removed the CI job name, thus the required checks will not be filled, this PR will need to be force merged by an administrator.

@@ -28,6 +28,19 @@
"jestConfig": "packages/common/jest.config.js",
"passWithNoTests": true
}
},
"version": {
"executor": "@jscutlery/semver:version",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@IamBlueSlime Do you have experience working with these plugin before as part of your day job? Or are these being included based just on the discussion we were having in #143

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I don't have any on these plugins, I just saw your discussion in the issue while doing this PR, so I've added them. But by reading their README's it seemed pretty accurate to our needs here :/

Copy link
Contributor

Choose a reason for hiding this comment

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

@IamBlueSlime Could you potentially give us some pro/cons of using this rather than other available plugins or ways you would achieve this? I haven used this plugin before but I'm kinda curious

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay no worries, I'll want to do some tests to make sure that the release and changelog process is smooth before we merge this in but the plugins definitely look promising

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@underfisk I never used Nx to publish packages, I'm afraid I can't give you any pro/cons about any package concerning publishing :/

Copy link
Collaborator

@WonderPanda WonderPanda left a comment

Choose a reason for hiding this comment

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

I'm not interested right now in switching away from Yarn 1 and over to NPM so we should revert the package-lock.json and changes to the CI scripts.

In the future I'd be more open to considering PNPM but it should be as a different PR and not mixed in with the NX stuff

@underfisk
Copy link
Contributor

I'm not interested right now in switching away from Yarn 1 and over to NPM so we should revert the package-lock.json and changes to the CI scripts.

In the future I'd be more open to considering PNPM but it should be as a different PR and not mixed in with the NX stuff

I agree, if we have to switch our package manager I would definitely advocate for PNPM but now on this scope, we should just aim to switch to Nx with the bare minimum of what we have, and then we can start thinking about what's next

@underfisk
Copy link
Contributor

@IamBlueSlime Can we merge the latest changes, i think we should start considering having Nx asap, there are some stuff that we have pending that can actually leverage having this ecosystem

@jeremylvln
Copy link
Contributor Author

@IamBlueSlime Can we merge the latest changes, i think we should start considering having Nx asap, there are some stuff that we have pending that can actually leverage having this ecosystem

Hi @underfisk! There is currently an issue with Nx that breaks JS reflection on build, see nrwl/nx#8784. I do not think that libraries are impacted, but only the apps that are build. In all cases, just to be sure, I think it would be preferable to wait until my PR on Nx is merged before merging this one.

But if you want me to rebase this PR and remove npm-related commits, I can totally do that :)

@underfisk
Copy link
Contributor

@IamBlueSlime Can we merge the latest changes, i think we should start considering having Nx asap, there are some stuff that we have pending that can actually leverage having this ecosystem

Hi @underfisk! There is currently an issue with Nx that breaks JS reflection on build, see nrwl/nx#8784. I do not think that libraries are impacted, but only the apps that are build. In all cases, just to be sure, I think it would be preferable to wait until my PR on Nx is merged before merging this one.

But if you want me to rebase this PR and remove npm-related commits, I can totally do that :)

No rush, i've ran into a similar problem before, well at max you can override terser plugin with a custom nx webpack file but if they can include that officially it would be better

@underfisk
Copy link
Contributor

@IamBlueSlime I've noticed that the Nx PR has been merged, any news so far if it is included in any of the latest versions?

@underfisk
Copy link
Contributor

@IamBlueSlime Do you have any plans on having this? I believe we were close to merge this experience

@jeremylvln
Copy link
Contributor Author

@IamBlueSlime Do you have any plans on having this? I believe we were close to merge this experience

Hey, sorry for the long time, I've been quite busy at work. I do not think I will have the time to work on this today, but maybe tomorrow or Wednesday. Is this okay for you?

@jeremylvln jeremylvln closed this by deleting the head repository May 7, 2023
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.

Convert to using NX Monorepo
3 participants