Skip to content

chore: tag packages with shared/core/plugin/tooling and set dep constraints #87

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

Merged
merged 3 commits into from
Oct 6, 2023

Conversation

matejchalk
Copy link
Collaborator

@matejchalk matejchalk commented Oct 4, 2023

Closes #28

Inspired by discussion in #75

Nx tags and project graph (updated)

image

Excalidraw link

@ChristopherPHolder
Copy link

Why are you creating a "shared" tag?

Isn't "utils" and "models" already a common way to structure a project?

I also don't think the CLI should be importing from "utils". Doing this means that the cli depends directly on the implementations in utils.

Currently it is only importing a couple of things which probably should be in core.

For example importModule , will this be used by a plugin? Or is this part of the core functionality?

Same with logErrorBeforeThrow and with CollectOptions

@matejchalk
Copy link
Collaborator Author

Why are you creating a "shared" tag?

Isn't "utils" and "models" already a common way to structure a project?

I also don't think the CLI should be importing from "utils". Doing this means that the cli depends directly on the implementations in utils.

Currently it is only importing a couple of things which probably should be in core.

For example importModule , will this be used by a plugin? Or is this part of the core functionality?

Same with logErrorBeforeThrow and with CollectOptions

My thinking was to separate core and plugin scopes, so everything that's available to both is shared.

What you're suggesting would require even more granularity in the tags. Basically, every package would need to have it's own dedicated tag, except for plugins.

In other words, instead of:

package tag
cli core
core core
utils shared
models shared
plugin-eslint plugin
plugin-lighthouse plugin
nx-plugin tooling

we would have:

package tag
cli cli
core core
utils utils
models models
plugin-eslint plugin
plugin-lighthouse plugin
nx-plugin nx-plugin

I originally wanted to avoid having many unique tags. I've previously thought of tags as a way to group projects by different dimensions (scope and type, usually), but maybe that only makes sense for applications. So if you think it's better to have single-purpose tags, then I can do it this way.

@ChristopherPHolder
Copy link

but maybe that only makes sense for applications.

The fact that we do not have the typical Apps, Lib structure, the CLI is an application and the rest are libs.

My thinking was to separate core and plugin scopes, so everything that's available to both is shared. What you're suggesting would require even more granularity in the tags.

I think you approach is great and i'm overthinking things.

I think is ready to merge, improves the codebase and is a step in the write direction.

But also i had a conversation with lars about tagin and lib types and what he said makes sense. We need to document things and make clear definitions. He also gave me this nice example.

I am not suggesting this is a priority for now. I am suggesting that once we get a better understanding of what makes sense for out style we add some nice docs.

@matejchalk
Copy link
Collaborator Author

matejchalk commented Oct 5, 2023

Yeah, it's quite possible we will be wiser about the project boundaries later on, so I'm all for going with a simple version for now and extending it later once it's clearer what we need.

Adding it to the project README is a good idea 👍 I'm happy to do that in this PR, or in a follow-up if you prefer (it's about time we replaced the generated Nx README anyway).

I really like the Greenforce monorepo example you shared, though 🙂 I've used the same 2D tags system suggested by Nx for several application monorepos (the portal also uses it), and this has even more types of libraries that could be useful. Unfortunately, I don't think a lot of them make sense for this repo, which is a lot smaller and simpler (and doesn't really have distinguishable UI, state, or backend layers).

But it got me thinking if we couldn't apply some of it after all 🤔 I would consider the tags I introduced to be scope tags, so we maybe could introduce the type tag dimension as well. Specifically, I think type:shell, type:feature and type:util (maybe also type:test-util or even type:e2e-util in the future) could apply for us:

project scope type
cli core shell
core core feature
utils shared util
models shared util
plugin-eslint plugin feature
plugin-lighthouse plugin feature
nx-plugin tooling shell

What do you think?

@ChristopherPHolder
Copy link

ChristopherPHolder commented Oct 5, 2023

I like your idea with the two dimensions, I think that what you currently have in that type should be what this MR ships.

Potentially i would change one detail. The cli is an application, maybe is should have the tag app.

I think that we should push the writing of the documentation to another MR and merge this one when you think its ready.

Note

I am not convinced that what i have noted from here on down makes any sense. I am writing down my thought here incase they do make sense (Probably Not?). Also this is not the priority so we should not think to much about this at the moment.

Applications

Type Description Allow Dependencies
app Entry point for the executable shell utils domain
app-e2e Runs E2E tests on the executable e2e-utils

Library types

Type Contains Allow Dependencies
shell Entry point for an application or domain. Orchestration and composition. util feature
domain Interfaces, types, constants, functions and services related to domain objects. domain util test-util
feature Specific business logic and application actions domain util
util Low-level utilities used by many libraries and applications (services, pure functions, constants). util

Notes

Again here i am making the distinction that the cli is an application, not a library.

By defining the Allowed Dependencies are specifying the scope.

The result would be this:

Project Type
cli app
core shell
utils util
models domain
plugin-eslint feature
plugin-lighthouse feature
nx-plugin shell

@matejchalk
Copy link
Collaborator Author

@ChristopherPHolder OK, I've made some changes based on what makes sense at this time. I recommended taking a look at the updated PR description.

I introduced the 2D tagging, although I just went with types app, feature, util and e2e.

I didn't go with domain because I don't think that distinction is useful here as we only have one "domain". And it would constrain us from importing types from models into utils. We have quite a few of those imports (e.g. we have report formatting logic in utils which needs the Report type), and I'm not sure refactoring it would be worthwhile (not much would be left in utils after that).

I agree with cli being more of an application than a library, especially now that we want the entry point to be executable. I changed the Nx project type as well to reflect it. But since this now makes it a type:app, I didn't see much value in having type:shell, so I just went with type:feature, same as for plugins.

@matejchalk matejchalk merged commit a9dc323 into main Oct 6, 2023
@matejchalk matejchalk deleted the nx-tags branch October 6, 2023 09:42
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.

Nx project tags and lint rule setup
2 participants