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

Jaybell/make buildable schematic #8916

Closed

Conversation

yharaskrik
Copy link
Contributor

Current Behavior

Cannot make libs buildable

Expected Behavior

Can now convert libs to be buildable

Related Issue(s)

Fixes #3427

@nx-cloud
Copy link

nx-cloud bot commented Feb 9, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit b50b692. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 21 targets

Sent with 💌 from NxCloud.

@vercel
Copy link

vercel bot commented Feb 9, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/FiDLX8QjUtnbY3u5pbBL97iipBKN
✅ Preview: https://nx-dev-git-fork-yharaskrik-jaybell-make-buildable-s-e55cea-nrwl.vercel.app

[Deployment for b50b692 canceled]

Copy link
Contributor

@Coly010 Coly010 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm going to tag some other vertical owners to have a look to ensure that it makes sense for their vertical.

Also, would you be able to write a test or two for this generator? That would be super helpful!

I've left some minor comments, but it looks good otherwise :)

@Coly010
Copy link
Contributor

Coly010 commented Feb 9, 2022

@FrozenPandaz @jaysoo Could you guys take a quick look over this to see if it makes sense to you too?

Comment on lines +39 to +41
if (configuration.targets['build'] != null) {
throw new Error(`${schema.project} is already buildable.`);
}
Copy link
Member

Choose a reason for hiding this comment

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

This check makes sense if we keep going with only single-project command. So this isn't necessarily a change request, but rather a discussion point. Should we change this generator to take --all similar to how it works with the convert-to-nx-project generator? Should it be able to take a list of projects instead of only 1?

Since buildable libs can only depend on other buildable libs, I feel like the most common DX will be converting all libs that are in an apps dependency tree to buildable at the same time.

Copy link
Contributor Author

@yharaskrik yharaskrik Feb 10, 2022

Choose a reason for hiding this comment

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

I would say yes we do want to be able to support that (and it's something I thought about while building this). Just a couple of things to consider:

  1. We would need to filter projects to only supported framework/type
  2. This Pr doesn't yet support Angular libs but that is handled by 1

"type": {
"description": "The type of library that this is.",
"type": "string",
"enum": ["js", "node", "nest", "next", "react", "detox", "web"]
Copy link
Collaborator

@FrozenPandaz FrozenPandaz Feb 10, 2022

Choose a reason for hiding this comment

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

Ideally, I think this would be built into each package:

nx g @nrwl/js:library lib1 --buildable would be equivalent to say like nx g @nrwl/js:library lib1 && nx g @nrwl/js:add-build lib1

  1. Having a switch statement here effectively means you're writing 7 generators in 1
  2. You use these generators to add the target configuration to any project similar to how @nrwl/jest:jest-project adds a test target onto an existing project.
  3. @nrwl/react:add-build would inherit @nrwl/web:add-build
  4. We already have the logic in our library generators, and only need to extract it to be reusable.

We have 4 executors (3 in this PR + angular) so I think this would probably wind up being 4 executors?

We used this kind of strategy for convert-to-eslint.

What do you all think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it might make more sense to have this split into each package. it would help keep like-minded code together, especially if we ever need to change how we create buildable libs and their config (e.g. angular might have to have some work done to the buildable lib implementation).

If the functionality to generate a buildable lib was extracted into a generator add-build that generator could just be reused in the library generator when --buildable is passed, as you suggested.

I like it, I think that makes more sense.

We could still have a generator in workspace package that will then call the underlying generators.

Copy link
Member

Choose a reason for hiding this comment

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

If we have these split into their respective packages, and have the root level 'add-buildable --all' option, how would it know run the generators? Should a generator in @nrwl/workspace rely on generators in other verticals? I wouldn't think so, but maybe dynamic imports would be a "good enough" way of doing them without adding deps to each plugin that supports buildable libs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking more along the lines of execSync(nx g ${packageName}:add-buildable)

@LayZeeDK
Copy link
Contributor

LayZeeDK commented Feb 22, 2022

Inspiration for tests and Angular support nx-worker/nxworker#5

@FrozenPandaz
Copy link
Collaborator

@yharaskrik thank you for taking a look at implementing this. This PR is pretty stale so I'm going to close it.

I think a good next step forward would be to focus on one type of library (e.g. Angular) and implement a nx g @nrwl/angular:convert-to-buildable generator and we can move one by one. Let's continue the discussion in the Nrwl Community slack if you have questions.

@yharaskrik
Copy link
Contributor Author

@yharaskrik thank you for taking a look at implementing this. This PR is pretty stale so I'm going to close it.

I think a good next step forward would be to focus on one type of library (e.g. Angular) and implement a nx g @nrwl/angular:convert-to-buildable generator and we can move one by one. Let's continue the discussion in the Nrwl Community slack if you have questions.

Hey Jason, ya no worries, i never got around to finishing this and don't think we came to a consensus on exactly how it should be implemented. If I find some time to tackle it again I will.

@expelledboy
Copy link

Just a heads up this is a very popularly requested feature, and the workflow for needing it is clear: create some libraries that are not buildable, create a new app in the future, need to update 5-20 packages!

Even just a step by step would be helpful.

@github-actions
Copy link

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 Mar 18, 2023
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.

Ability to make an existing library buildable
6 participants