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

Way to detect use of dev dependency #1049

Closed
marc-mabe opened this issue Dec 6, 2022 · 5 comments · Fixed by #1174
Closed

Way to detect use of dev dependency #1049

marc-mabe opened this issue Dec 6, 2022 · 5 comments · Fixed by #1174

Comments

@marc-mabe
Copy link

Most projects are using composer with defined dependencies as well as dependencies for development only.
In a normal case you will install all dependencies (dev and no-dev) on running your tests. Then on building/deploying you either remove the dev dependencies and you build a new installation without dev dependencies.

As a result on local development nobody will stop you using something which is available on development only and this will not even get detected on running tests so it fails on deployment or even worse on live.

It would be great to be able to collect all dev dependencies and so be able to disallow them.
Something like this:

deptrac:
    layers:
    - name: dependencies-dev
      collectors:
        - type: composer
          value: require-dev

Thoughts ?

@dbrumann
Copy link
Collaborator

dbrumann commented Dec 6, 2022

I get the problem you are talking about. I am not sure the suggested collector is that helpful. It seems overly broad to me.

For example, in a Symfony project you might have Doctrine AppFixtures in the src/ directory, but those classes are only used in dev and can use all dev-dependencies, but likely only should use the ORM-Fixtures. Other dev-dependencies might only be relevant in your tests, but again this is probably just a subset not all of the dev-dependencies.

Instead of your collector, you could use directory or class and then add all relevant dev-dependencies to that layer. That way, you can be more specific, e.g. allow a certain dev-dependency to be used in certain layers.

@dbrumann
Copy link
Collaborator

dbrumann commented Dec 6, 2022

The main problem I see with this collector, is that it only scratches the surface. You probably want not just all dependencies explicitly marked as require-dev, but also transitive dependencies, which quickly makes this collector become really complex.

I'll leave this open for now, in case someone has a good suggestion how to tackle this.

@marc-mabe
Copy link
Author

Hi @dbrumann

For example, in a Symfony project you might have Doctrine AppFixtures in the src/ directory, but those classes are only used in dev and can use all dev-dependencies, but likely only should use the ORM-Fixtures.

I don't think it's a good idea in general to mix dev and production logic.
Where this is not 100% separated it can still be configured to be ignored by case-by-case basis.

Other dev-dependencies might only be relevant in your tests, but again this is probably just a subset not all of the dev-dependencies.

True, where defining an allowed route for tests to dev dependencies should be trivial I think

Instead of your collector, you could use directory or class and then add all relevant dev-dependencies to that layer. That way, you can be more specific, e.g. allow a certain dev-dependency to be used in certain layers.
...
The main problem I see with this collector, is that it only scratches the surface. You probably want not just all dependencies explicitly marked as require-dev, but also transitive dependencies, which quickly makes this collector become really complex.

As you noticed already it would only be useful with require-dev + transitive dev-only dependencies.
So managing all the directories manually is a no-go.

Not sure how complex such an collector would be ... I'm coming from https:/j6s/phparch which had this feature based on composer.lock packages-dev. I thought based on the defined file path the package where it's coming from can be detected and based on that the collector could build up an inner collector based on directories.
-> list packages-dev packages from composer.lock -> build directories ala {vendor-dir}/{vendor-name}/{vendor-package}

@patrickkusebauch
Copy link
Collaborator

I think this is possible, but quite an undertaking. Looking at the structure of the composer.lock file, you get the transitive dependencies of your first class (read as in composer.json defined) dependencies. So it could be done. I would like to see to a configuration more in terms of package names, rather than require/require-dev, so like this:

deptrac:
    layers:
    - name: dependencies-dev
      collectors:
        - type: composer
          value:
           - org1/package1
           - org2/package2

That way it can also be used for the production dependencies and you could limit which library can be used in which part of the code. It gives you a lot of the granularity you are both mentioning it would need to work correctly.

@patrickkusebauch
Copy link
Collaborator

patrickkusebauch commented Mar 11, 2023

A follow-up on the previous thought. If you combine using the aforementioned config with the --fail-on-uncovered CLI flag, you will force yourself to explicitly define all composer dependencies. They are the last bastion of dependencies that is not (usually) assigned to layers. In combination with the private collectors you can also specify where exactly which composer dependencies are allowed to be used. For example:

deptrac:
    layers:
    - name: Fixtures
      collectors:
        - type: className
          value: "App/orm-fixtures/*"
        - type: composer
          value:
           - ORM/orm-fixtures
          private: true 

Such configuration would enforce that your dev dependency on ORM/orm-fixtures is only used in your App/orm-fixtures code that is not running on production.

Another (tangential) benefit of this feature would be that we could have a rule on transitive dependencies. If you specify the path to your composer.lock file, we can mine it for all the classes that are loaded as primary dependencies and transitive dependencies. With this information, we can still keep primary dependencies as (un)covered while transitive dependencies would always be a violation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants