context:

i’m curious as to why it’s considered bad practice to directly export something using importCond if it does work in production?

i could be wrong, but i feel like having a feature gate to control which component is used by dependents would be a semi common pattern, and having the additional function call to get the component makes code harder to read/understand, and puts us in a position where people may use wildly different names for a component, instead of at least needing to reference the component name?

e.g.:

/// @filename: foo-component.tsx
const FooComponent = importCond<typeof import('./old.tsx'), typeof import('./new.tsx')>(
    'gate-name',
    './old.tsx',
    './new.tsx',
);
export const getFooComponent = () => FooComponent;
/// @filename: bar-component.tsx
import { getFooComponent } from './foo-component';
// we can rename it here
const BarComponent = getFooComponent();

Exporting importCond function calls will cause issues in test environments, so we don't want to encourage them.

As for why we made this decision, we have a couple of reasons:

• Moving the importCond means making the fg call not colocated into the relevant file for static analysis
• Another form of barrel files (with similar costs to bundling and bundle size)
• We don't want to encourage people to introduce code like in your example. Exporting an importCond call, even implicitly, means that we can't bundle split as efficiently. Each call to importCond is an opportunity to bundle split.

question: / suggestion:

is type information required to run this rule? if not, we can probably remove the tsconfig/project services options to make running unit tests a bit faster?

oh you're right, none of these options are actually needed 👀

i had added this (and filename) because i was running into trouble getting typescript-eslint to work in this repo and this seemed to help, but i can't actually reproduce the issues i was running into anymore 🤔 so they must not have been necessary after all

suggestion:

the semver >8.0.0 will match anything greater than 8.0.0, including 10.0.0 (which could have major breaking changes).

it would be considerably safer to use the range ^8.57.0 || ^9.0.0 to ensure that they only use a version that supports all the new properties in eslint rules (the ones mentioned here https://eslint.org/blog/2023/09/preparing-custom-rules-eslint-v9/ - while they are able to be used in in i believe 8.49.0, there might be some other things added between the versions? i can't remember)

suggestion:

i would recommend using (or at least testing with) eslint v9 for the eslint plugin, even if it's a separate test (since that is what jira is using) if possible? :)

i realise it's probably difficult to update the whole repo to using eslint v9, and likely not in scope for this task, because eslint-plugin-flowtype and eslint-plugin-monorepo haven't been updated in ages, but i thought it might be useful to mention?

you may be able to get away with using @eslint/compat and using fixupPluginRules though?

i will probably leave moving to eslint v9 for a future PR to be honest 😅 as it would involve checking several other packages around the repo (at least, i couldn't complete the version upgrade within a one-hour timebox)

i wonder if it's possible to set up this repo to test with eslint v9 just for the @atlaspack/eslint-plugin package? 🤔

I did confirm that the tests added in this PR do work locally when i change the whole repo to eslint v9, so integrating into jira would be fine 😅

Nice one

Exporting importCond function calls will cause issues in test environments, so we don't want to encourage them.

As for why we made this decision, we have a couple of reasons:

• Moving the importCond means making the fg call not colocated into the relevant file for static analysis
• Another form of barrel files (with similar costs to bundling and bundle size)
• We don't want to encourage people to introduce code like in your example. Exporting an importCond call, even implicitly, means that we can't bundle split as efficiently. Each call to importCond is an opportunity to bundle split.