search

wix-incubator / tslint-wix-react

Linting configuration on top of tslint-microsoft-contrib for tsx projects
0 stars 3 forks source link

no-submodule-imports #8

Open liorzisman opened 6 years ago

liorzisman commented 6 years ago

In the upcoming release v5.7.0 it is allowed to whitelist submodules like @angular/core/testing.

After mistakenly trying to npm i i got the new version, and got a linting error on react-dom/server in the ssr.node-spec.tsx. Still need to further investigate, but I feel they added the ability to whitelist submodules because by default now they all fall under 'no-submodule-imports' rule :)

After discussing with @AviVahl, decided to open this issue to consider what to do, as we do a lot of submodule imports in tests (for example).

AviVahl commented 6 years ago

We need to decide whether we want this rule enabled by default. If we do, that would mean we need to whitelist any alternative entry point that we use, like for React SSR.

@VKobeliatsky @tyv

AviVahl commented 6 years ago

https://github.com/wix/stylable-components/commit/e7e8b394c685ffe879d4fb820997fa8cdff9e8c2

tyv commented 6 years ago

I don't see any reason to have this rule enabled. I vote to disable it.

AviVahl commented 6 years ago

II actually ended up liking it. It already caught cases where people went into ./dist/src/... in stylable-react-component.

People should never target internals. Only public entry points.

For the few cases where a package has an alternative entry point (like react or kissfs), it forces you to explicitly allow it.

Overall, better control and making it harder to hack the internals.

tyv commented 6 years ago

@AviVahl the cost is support price, good when this rule is common for all and placed here, but we can't whitelist everything here and release new version each time when one project need smth to be whitelisted.

I think we can leave this rule off here in common place, but we can turn it on at the project level with appropriate whitelist

AviVahl commented 6 years ago

I want to be notified if some file imports internals of another package. Most forward compatibility gaps we had for upgrading packages in the past was people hacking away internals.

tslint 5.7+ turned it on by default, so there's nothing to "leave off".

I agree we could explicitly turn it off in the main preset, but that would require a bit more verbose reasoning than "support price". I took me less than 30 secs to fix stylable-components once I wanted to upgrade tslint, while this was one of those rules which gave me development benefit right away,

I vote to leave it with the default tslint behavior, and white-list specific alternative entry points (per project). There really aren't that many.

tyv commented 6 years ago

good, I don't mind

VKobeliatsky commented 6 years ago

so, do we vote on this?