softarc-consulting / sheriff

Lightweight Modularity for TypeScript Projects
MIT License
183 stars 14 forks source link

Nx Lib Index.ts #66

Closed rainerhahnekamp closed 5 months ago

rainerhahnekamp commented 7 months ago
import { noDependencies, sameTag, SheriffConfig } from '@softarc/sheriff-core';

export const sheriffConfig: SheriffConfig = {
  tagging: {
    'libs': {
      'shared/src': 'nx-lib',
      'shared/src/lib/<shared>': 'shared',
      '<domain>/src': 'nx-lib',
      '<domain>/src/lib/feat-<name>': ['domain:<domain>', 'type:feature'],
      '<domain>/src/lib/<type>': ['domain:<domain>', 'type:<type>'],
    },
  },
  depRules: {
    root: ['type:api', 'shared', ({ to }) => to.startsWith('domain')],
    'domain:*': [sameTag, 'shared'],
    'type:api': ['type:feature', 'type:model', 'type:ui', 'type:data'],
    'type:feature': ['type:model', 'type:ui', 'type:data'],
    'type:ui': ['type:model'],
    'type:data': ['type:model'],
    'type:model': noDependencies,
    shared: 'shared',
  },
};

Fails with

       /Users/rainerh/programming/powerduo-nx-and-sheriff/sheriff-nx/apps/eternal/src/app/app.config.ts
         1:1  error  Dependency Rule (internal error): placeholder for value "domain" does already exist  @softarc/sheriff/dependency-rule
attilacsanyi commented 6 months ago

This is when an extra index.ts is defined somewhere in the folder structure. Am I right @rainerhahnekamp ?

rainerhahnekamp commented 6 months ago

Hi @attilacsanyi, I honestly don't know. I just encountered and filed this bug so I don't forget it. It could be as you said.

Do you have this error in your project?

attilacsanyi commented 6 months ago

Hi @attilacsanyi, I honestly don't know. I just encountered and filed this bug so I don't forget it. It could be as you said.

Do you have this error in your project?

Yes I got this if I introduce a new index.ts lets say in a components subfolder of src in an Nx lib, beside the library root index.ts

So if I add an empty index.ts (marked as red) I got the following error: error Dependency Rule (internal error): placeholder for value "type" does already exist @softarc/sheriff/dependency-rule

image

I hope this helps @rainerhahnekamp 🤞 thanks for any hint on this, meanwhile I stick to only using one index.ts per lib

rainerhahnekamp commented 6 months ago

Thanks, that helps. Let me try to find a fix for that.

rainerhahnekamp commented 6 months ago

Update: The error has been found, and a fix has also been provided. However, there is one edge case that I want to verify before I push and release.

attilacsanyi commented 6 months ago

Update: The error has been found, and a fix has also been provided. However, there is one edge case that I want to verify before I push and release.

Thank great, thanks @rainerhahnekamp which version have the fix?

rainerhahnekamp commented 6 months ago

Hi @attilacsanyi, fix has been merged and published in version 0.14.4. It would be great if you can check it out and confirm that it works on your machine.

attilacsanyi commented 6 months ago

Hi @attilacsanyi, fix has been merged and published in version 0.14.4. It would be great if you can check it out and confirm that it works on your machine.

It seems working I got more specific lint error for the above shared components structure:

error  Dependency Rule (internal error):
No assigned Tag for '.../libs/shared/ui/src/lib/components' in sheriff.config.ts  @softarc/sheriff/dependency-rule

This is expected as I have tags only in lib root index barrel in Nx workspace:

// sheriff.config.ts
...
export const sheriffConfig: SheriffConfig = {
  tagging: {
    libs: {
      'shared/<type>/src': 'shared:<type>',
      ...
    },
  },
  depRules: {
     ...
  },
};
...
rainerhahnekamp commented 6 months ago

Yes, you have to tag every module. There is no way around it. I've discussed mitigation strategies in #69.

I'm still wondering if it would be better not to call it "internal error."" Internal error" sounds like a bug of sheriff itself.

attilacsanyi commented 6 months ago

Yes, you have to tag every module. There is no way around it. I've discussed mitigation strategies in #69.

I'm still wondering if it would be better not to call it "internal error."" Internal error" sounds like a bug of sheriff itself.

Maybe "Missing configuration" would be better, but I am fine as I see a more detailed message about the missing tag in the above case.

I also think it's a good idea to keep only one index.ts in the root of the libs, just simpler and less tags and rules needed to be handled. The rest should be relative imports.

Thanks for the quick fix.

rainerhahnekamp commented 6 months ago

Yes, but I think we should rename those kinds of errors to "user error" or similar.

If you use NX and just have one index.ts per lib, then I wouldn't recommend using Sheriff.

Sheriff was made to have multiple modules inside an Nx library. As long as you stick to conventions and every module has the same folder structure, you should be able to achieve that without making it too complicated.

The other option would be modules with index.ts as discussed here #47

attilacsanyi commented 6 months ago

Hmmm, "If you use NX and just have one index.ts per lib, then I wouldn't recommend using Sheriff."

I want to achieve separated libs and Sheriff help me to auto tag projects based on the folder naming convention. So I can skip the manually maintained tags in every Nx project as I would do normally. Then I create the depRules as I need to allow only specific imports from one library into another.

With this config I have one index.ts barrel per library like in the official Nx Boundaries. I want to utilise the Nx DTE hence I separated out those libraries by domains and types as below:

This version inspired by your full-nx version of Sheriff config.

// sheriff.config.ts
export const sheriffConfig: SheriffConfig = {
  tagging: {
    libs: {
      'shared/<type>/src': 'shared:<type>',
      '<domain>/feat-<name>/src': ['domain:<domain>', 'type:feat'],
      '<domain>/<type>/src': ['domain:<domain>', 'type:<type>'],
    },
  },
  depRules: {
    // app should use any ui lib and any shared
    root: ['type:api', 'shared:*', '*:ui', ({ to }) => to.startsWith('domain')],
    'domain:*': [sameTag],
    // api can use any type from the same domain
    'type:api': ['type:*'],
    'type:feat': ['type:ui', 'type:data', 'type:model', 'shared:*'],
    // ui can use shared ui and model
    'type:ui': ['type:model', 'shared:ui', 'shared:model'],
    // data can use shared data and model
    'type:data': ['type:model', 'shared:data', 'shared:model'],
    'type:model': ['shared:model'],
    'shared:ui': ['shared:model'],
    'shared:data': ['shared:model'],
    'shared:model': noDependencies,
    'shared:*': [sameTag],
  },
};

Appreciate your feedback why this is not recommended in Nx monorepo with multiple libs and what else do I need to achieve the same as above in Sheriff config. @rainerhahnekamp

attilacsanyi commented 5 months ago

Hmmm, "If you use NX and just have one index.ts per lib, then I wouldn't recommend using Sheriff."

I want to achieve separated libs and Sheriff help me to auto tag projects based on the folder naming convention. So I can skip the manually maintained tags in every Nx project as I would do normally. Then I create the depRules as I need to allow only specific imports from one library into another.

With this config I have one index.ts barrel per library like in the official Nx Boundaries. I want to utilise the Nx DTE hence I separated out those libraries by domains and types as below:

This version inspired by your full-nx version of Sheriff config.

// sheriff.config.ts
export const sheriffConfig: SheriffConfig = {
  tagging: {
    libs: {
      'shared/<type>/src': 'shared:<type>',
      '<domain>/feat-<name>/src': ['domain:<domain>', 'type:feat'],
      '<domain>/<type>/src': ['domain:<domain>', 'type:<type>'],
    },
  },
  depRules: {
    // app should use any ui lib and any shared
    root: ['type:api', 'shared:*', '*:ui', ({ to }) => to.startsWith('domain')],
    'domain:*': [sameTag],
    // api can use any type from the same domain
    'type:api': ['type:*'],
    'type:feat': ['type:ui', 'type:data', 'type:model', 'shared:*'],
    // ui can use shared ui and model
    'type:ui': ['type:model', 'shared:ui', 'shared:model'],
    // data can use shared data and model
    'type:data': ['type:model', 'shared:data', 'shared:model'],
    'type:model': ['shared:model'],
    'shared:ui': ['shared:model'],
    'shared:data': ['shared:model'],
    'shared:model': noDependencies,
    'shared:*': [sameTag],
  },
};

Appreciate your feedback why this is not recommended in Nx monorepo with multiple libs and what else do I need to achieve the same as above in Sheriff config. @rainerhahnekamp

@rainerhahnekamp I appreciate your feedback on this, thank you so much.

rainerhahnekamp commented 5 months ago

Sorry Attila, missed to answer.

I honestly wasn't aware of your use case. I always thought that Sheriff only becomes interesting, when you want modules inside an Nx lib or module checks in the Angular CLI

I didn't see the convenience of the sheriff.config.ts as reason enough to go with Sheriff, so that's actually very nice to hear.

attilacsanyi commented 5 months ago

Sorry Attila, missed to answer.

I honestly wasn't aware of your use case. I always thought that Sheriff only becomes interesting, when you want modules inside an Nx lib or module checks in the Angular CLI

I didn't see the convenience of the sheriff.config.ts as reason enough to go with Sheriff, so that's actually very nice to hear.

Thanks for your feedback @rainerhahnekamp , yes it seems Sheriff is so flexible that we can configure it in different ways.

  1. folders inside one Nx lib with index.ts barrels: sheriff-nx
  2. each Nx lib is the module itself without extra folder barrels full-nx Still try to understand when 1 .is better over 2. approach or vica - versa.

I spotted in your your full-nx version of Sheriff example that manually adding tags in project.json is not needed at all in order to make Sheriff logic working.

Those tags are needed only if we want to utilize Nx boundaries eslint rule, but it is less flexible than Sheriff folder structure pattern mapping and kind of redundant as Sheriff do similar things already.