[eslint-plugin-packlets] Check imports from tsconfig#compilerOptions.paths

[trane]

Fixes #3121

Summary

The packlets plugin checks local imports to ensure that we only import local code from other packlets — but there is a common edge case in Typescript repositories for which this fails: imports that use aliases defined in the tsconfig.json's compilerOptions.paths. This change extends the packlets plugin definition of a local import to include Typescript path aliases.

Details

Imagine this directory structure:

repo
├── app
└── src
    └── packlets
        ├── logger
        └── metrics

Now take this import statment in src/packlets/logger/log.ts:

import { Thing } from '../../app' // ERROR

However, if your tsconfig.json had the following configuration:

{
  "compilerOptions": {
    "paths": {
      "@app": ["app"],
    }
  }
}

An alias in your src/packlets/logger/log.ts would pass linting:

import { Thing } from '@app' // ALLOWED

It is also common to have aliases for node_modules and packlets themselves, so this filters out path aliases for these cases. So, take this more worked tsconfig.json example:

{
  "compilerOptions": {
    "paths": {
      "@app": ["app"],
      "@myorg/packlets/*": ["src/packlets/*"],
      "jquery": ["node_modules/jquery/dist/jquery"],
    }
  }
}

The following would happen to these imports in your src/packlets/logger/log.ts:

import { Thing } from '@app' // ERROR
import * as jquery from 'jquery' // ALLOWED
import * as metrics from '@myorg/packlets/metrics' // ALLOWED

Details

Alternatives

I considered a few alternatives (or extensions):

1. Don't look at tsconfig.json, but allow configuration of the plugin which specifies a list of import path prefixes that should be seen as "local". The downside here is that additional maintenance is required for repo maintainers when compilerOptions.paths gets updated they must also remember to update the plugin configuration.
2. Extend my implementation with configuration to specify which aliases should be not be seen as "local". Since my implementation hard-codes both src/packlets and node_modules as ignored, this is in the spirit of this change.
3. Extend my implementation with configuration allowing users to configure which of the compilerOptions.paths aliases they want to opt-in to the linter — explicitly failing the linting step if the alias is not present. This reduces the maintenance overhead of alternative (1), making it so there isn't a silent failure.

Missing cases

This implementation ignores compilerOptions.paths which contain src/packlets and node_modules, but, it is likely that users will want to opt-out of more than these paths. This could be achieved by alternative (2).

Backwards compatibility

It will cause previously hidden local imports to be surfaced in codebases.

How it was tested

• I added some unit tests for the .isModulePathAnAlias function
• I ran the plugin against a large internal codebase where we are using packlets as an intermediate step towards npm modules. It found all the expected issues in the initial packlets we created where they imported using a path alias.

[trane]

I thought I'd check in on this PR to see if there is any additional work I can do to make it ready to merge.

[iclanton]

Note: PR #3337 has renamed our GitHub master branch to main. This should not affect your pull request, but we recommend to redo your git clone to avoid confusion with the old branch name.

[zouxuoz]

@iclanton do we have a chance to merge this PR? Looks like branch doesn't have conflicts with main branch

This update will be helpful for our project

I can help with PR if we need to update/refresh something

[octogonz]

Should we consider other equivalent forms such as ./src/packlets instead of src/packlets?

[octogonz]

Also IIRC baseUrl affects the interpretation of these paths. (If so it would be okay to simply report an error if baseUrl is present rather than trying to implement the full semantics.)

[octogonz]

Seems like path.startsWith('src/packlets') would incorrectly match src/packletsDocs/index.ts

[octogonz]

In my experience, tsconfig.json paths seem to be a poor practice for large scale code bases, due to the following chain of reasoning:

• If you make a library package with tsconfig.json path mappings, then in general the resulting .d.ts files will have imports that reference mapped paths.
• But library consumers will have their own different tsconfig.json that cannot see the tsconfig.json of your library; thus the mapped paths will malfunction unless the consumer manually redeclares every path mapping of every library, which is a clumsy developer experience.
• These considerations do not apply to a standalone app project. However, as your app gets larger, it is natural to refactor by extracting subsets of the app code into separate library projects. Therefore if we apply different engineering standards to apps vs libraries, it discourages refactoring and leads to oversized monolithic projects.

That said, we're happy to accept this feature if it doesn't break anything, since not every code base is a large scale code base. 🙂

[octogonz]

I can help with PR if we need to update/refresh something

@zouxuoz are you still available to help? We can merge this PR if someone can fix the path-calculation bugs that I pointed out.