Review usage of imports and be consistent for external vs internal dependency sequence.

[nerrad]

In one of my recent pulls, @mikejolley noted that I had been defining internal dependencies before external dependencies. I didn't even realize I was doing it wrong and it should be external dependencies defined before internal dependencies.

This issue is to make sure a review is done to fix the inconsistency (and maybe a good opportunity to add a lint rule?)

[senadir]

should be external, woocommerce and internal, or simply external and internal, external is confusing since a lot of our internal code is aliased to an "external name"

[Aljullu]

should be external, woocommerce and internal, or simply external and internal, external is confusing since a lot of our internal code is aliased to an "external name"

For reference, that was discussed some months ago in #1127 (comment).

With time, we got more and more @wocommerce packages, so it might make sense to revisit it if needed.

[nerrad]

My opinion hasn't changed much from #1127. The question I ask is, what value do we get from adding another code block for @woocommerce/* aliases/packages? The only slight benefit I could see is that we see all woocommerce packages/aliases grouped together.

In my opinion, the only thing that really matters from a code review (cursory glance) standpoint is whether an import is relative or "external" (either via node_modules import, external reference, or aliased reference).

I'm wary of adding additional groupings that introduce more code churn in existing code and additional overhead in writing imports (for negligible benefit).

With that said, I think the woocommerce/admin repository does group @woocommerce dependencies on their own block (I'm just not sure how consistent that is...). So this is another are we may need some discussion with other teams to align on conventions/standards for imports in Woo js.

[mikejolley]

I'm not sure on which side I sit regarding a section for @woocommerce/* dependencies, but I do know that unless someone is smart enough to make a script to do this in bulk, it would be a massive change with very little benefit to us. If anything, it would need to be added as a linting rule and updated as we touch files.

I'm happy to find the external/internal violations though :)

[mikejolley]

Found using regex Internal dependencies((.|\n)*)External dependencies

[nerrad]

I'm not sure on which side I sit regarding a section for @woocommerce/* dependencies, but I do know that unless someone is smart enough to make a script to do this in bulk, it would be a massive change with very little benefit to us. If anything, it would need to be added as a linting rule and updated as we touch files.

💯

[mikejolley]

Given us the option in #2775

[nerrad]

Implementing #2957 will fix this so I'm going to close this issue in favor of the other.