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

Allow for tags to be applicable to only certain files #90

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ahlec
Copy link

@ahlec ahlec commented Feb 10, 2021

Description

Currently, tags are universally applicable to all files in the tree of the fence.json file. This works well for smaller projects or for repositories that make use of directories, but can be limiting for flatter directory hierarchies. Since imports and exports are powered off of tags, it can be problematic when you want to permit the importing of one or two select files, without allowing the entire directory.

For example:

src/
  application-1/
    fence.json     <— Good with importing anything from shared

  components/
    fence.json     <— Wants to allow shared/theme.ts, but would require also allowing shared/redux

  shared/
    redux/
      ...

    theme.ts

For this, I took the mentality of specifying that certain dependencies were only allowed for specific files, and added the same logic to tags. By default, all string tags operate like they did — globally applicable to the entire tree of the config file. But you can now also specify them as an object, providing a glob path/paths to allow for tagging only specific files:

{
  "tags": [
      "shared",
      {
        "applicableTo": "theme.ts",
        "tag": "theme"
      }
  ]
}

Doing this, components/ could now just import shared/theme.ts — it will still be an error to import shared/redux.

Testing

I've updated the unit tests and wrote some new ones concerning tag resolution and how getTagsForFile works. I've also updated the sample application to showcase an example of this.

Areas of Concern

Primarily, I'd want to make sure that my logic for applying the glob pattern relative to the directory the config file was defined in makes sense. Programmatically it runs, but it might make some assumptions that I'd like to make sure are actually valid.

Also, let me know if there are any stylistic or formatting issues.

@smikula
Copy link
Owner

smikula commented Feb 11, 2021

@ahlec — I'm curious, is it that difficult to move the files you want to share (theme.js, in this case) into its own directory? My aim is a simple mental model where tags correspond to directories. I'm having a hard time imagining a case where that isn't adequate, unless for some reason you really can't move code into a subdirectory.

@ahlec
Copy link
Author

ahlec commented Feb 11, 2021

theme.ts could be moved to its own directory at shared/theme/theme, but overall that would lead to a lot of one-item directories. Files that were already their smallest possible version (ie, there's not enough to spread across multiple files in a directory) would be alone in that directory and you'd be forced either to double-specify the name (once for the directory, once for the file) or you'll have a ton of index files.

I think it also helps with building tighter fences as well. For instance, many of the codebases I work on have unit test files alongside their React components. When I'm building the fence files, I need to permit all files in the hierarchy to be allowed to import enzyme or jest, even though really I only want to allow the unit test files to import that. With something like this, I'd be able to set up a fence like this (in theory? I haven't tested it yet tbh):

{
  "tags": [
    "components",
    {
      "applicableTo": "**/*.unit.tsx",
      "tag": "unit-tests"
    }
  ],
  "dependencies": [
    "react",
    {
      "accessibleTo": "unit-tests",
      "dependency": "enzyme"
    }
  ]
}

@smikula
Copy link
Owner

smikula commented Feb 11, 2021

theme.ts could be moved to its own directory at shared/theme/theme, but overall that would lead to a lot of one-item directories. Files that were already their smallest possible version (ie, there's not enough to spread across multiple files in a directory) would be alone in that directory and you'd be forced either to double-specify the name (once for the directory, once for the file) or you'll have a ton of index files.

I think it also helps with building tighter fences as well. For instance, many of the codebases I work on have unit test files alongside their React components. When I'm building the fence files, I need to permit all files in the hierarchy to be allowed to import enzyme or jest, even though really I only want to allow the unit test files to import that. With something like this, I'd be able to set up a fence like this (in theory? I haven't tested it yet tbh):

That makes sense. It doesn't hurt to have the option for those who want to use it; I just might not promote it as the the typical approach.

I'm pretty busy with other stuff at the moment, but I'll keep this PR around and try to get it in when I have a chance.

@ahlec
Copy link
Author

ahlec commented Feb 11, 2021

Thank you, and no rush!! And certainly, if you'd like this implemented a different way, broken apart, or even if you don't want to take it, that's fine; I don't want to shoehorn something in if it breaks how you view the tool overall. This just felt to me like something I kept wanting to reach for in some advanced cases, and felt like a natural pairing for the advanced dependencies control. But I'm super happy to work with you if you want to iterate on this some more.

@ahlec
Copy link
Author

ahlec commented May 25, 2021

Hey! I'd wanted to check in if there was any progress on this front/if you'd had time to think about this proposal?

@smikula
Copy link
Owner

smikula commented May 26, 2021

@ahlec Sorry, yes, my intent is to take this; I just want to take a close look at it before I merge it and it's hard for me to carve out time for that.

@ahlec
Copy link
Author

ahlec commented Aug 27, 2021

Wanted to touch base again on this PR since I've run in to some more use cases for this the other day.

@ahlec
Copy link
Author

ahlec commented Oct 23, 2021

Is there anything I'd be able to do to help move this along?

@smikula
Copy link
Owner

smikula commented Oct 25, 2021

Is there anything I'd be able to do to help move this along?

No, sorry, I've been letting it linger. It's tough to prioritize since I'm swamped enough with my day-job priorities. But it's back on my radar, so I'll see if I can squeeze it in.

@czlowiek488
Copy link

Is someone who would like finish and merge this PR?
I would like to have this exact feature in my app.
I have this file structure.

features/
   featureName/
      actionA/
         handler.ts
         handler.test.ts
      actionB/
        handler.ts
        handler.test.ts
tests/
    generic.ts
    ...

Files with .test extension import files from ./tests.
What I want to achieve is disallowing to import from ./tests for files without .test extension.

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.

3 participants