Consider moving client/extensions to extensions

[coderkevin]

For context, this was started by a comment from @gziolo and other comment from @blowery not favoring the way we deal with root import context currently.

This is assuming we want to keep the ability to import relative to an extension in Calypso (e.g. import X from 'woocommerce/state/action-types' instead of import X from '../../../../action-types')

If so, then it may make the case for moving client/extensions to extensions so that way we don't have nested import root contexts.

I'm logging this issue here with the intent of furthering discussion on this topic.

[coderkevin]

@ehg commented (on #14731):

So we really want to rename client to app, as we're already using isomorphic code. The problem will be what to rename server. So extensions should live within client for now.

I think the changes look good here. My concern would be bloating the server bundle, as we add more extensions — is this a valid concern? I haven't looked too deeply yet.

Also relevant (from @mtias): #2319

[mtias]

With renaming client to app I concur that extensions should not be moved are fine where they are.

[gziolo]

Interesting fact: how they deal with shared code at Facebook.

Even though Haste (with Haste all filenames are globally unique) allows us to import any module from anywhere in the repository, we follow a convention to avoid cyclic dependencies and other unpleasant surprises. By convention, a file may only import files in the same folder or in subfolders below.

For example, files inside src/renderers/dom/stack/client may import other files in the same folder or any folder below.

However they can't import modules from src/renderers/dom/stack/server because it is not a child directory of src/renderers/dom/stack/client.

There is an exception to this rule. Sometimes we do need to share functionality between two groups of modules. In this case, we hoist the shared module up to a folder called shared inside the closest common ancestor folder of the modules that need to rely on it.

For example, code shared between src/renderers/dom/stack/client and src/renderers/dom/stack/server lives in src/renderers/dom/shared.

By the same logic, if src/renderers/dom/stack/client needs to share a utility with something in src/renderers/native, this utility would be in src/renderers/shared.

This convention is not enforced but we check for it during a pull request review.

[gziolo]

I double checked usages like import { LOADING } from 'woocommerce/state/constants';. It looks like it would be even more convenience to teach Webpack to change module resolution system to scan imports inside extensions this way:

// file: client/extensions/woocommerce/state/sites/shipping-methods/reducer.js
import { LOADING } from 'state/constants';

would search for file in this order for client:

• client/extensions/woocommerce/state/constants
• client/state/constants
• node_modules/state/constants

would search for file in this order for server:

• client/extensions/woocommerce/state/constants
• server/state/constants
• client/state/constants
• node_modules/state/constants

This would ensure that code inside extension takes always precedence over all global code. This would also help us to mitigate the issue where extension developers import code from other extensions.

I assume it is doable with Webpack given its flexibility. Unfortunately it wouldn't be as easy to setup with Mocha.

[gziolo]

BTW, should we close this and continue in #2319?

[coderkevin]

I don't personally have any issue with importing from woocommerce/state/constants for example. I'd be a little scared of implementing it from the extension name as root mostly because we'd shadow existing import paths. For example, if we did state/action-types it would always resolve to client/extensions/woocommerce/state/action-types and we might want client/state/action-types instead.

[coderkevin]

BTW, one of the things I'm watching for on this is compatibility with non-wpcom implementations of this. Eventually, we'll likely want to be able to run the same extension code either within wpcom or outside of it. We'll need to be able to support imports like this in both environments.

[coderkevin]

BTW, should we close this and continue in #2319?

I see these as two separate issues that could be solved by the same PR. We've got them linked, so we'll keep track, but I think the comment threads can continue separately.

[gziolo]

I don't personally have any issue with importing from woocommerce/state/constants for example. I'd be a little scared of implementing it from the extension name as root mostly because we'd shadow existing import paths. For example, if we did state/action-types it would always resolve to client/extensions/woocommerce/state/action-types and we might want client/state/action-types instead.

This limitation works the other way around, too. If we pick the same name for extension as the existing subfolder in client/ we can have a very similar collision. However it doesn't make any difference where we put extensions folder, we still would have the same issue. We could investigate if Lerna would helps us those kind of issues. I guess it would allow to leave project's structure as it is because you could specify packages this way:

{
  "packages": [ "client/extensions/*", "client/*", "server/*" ]
}

I'm not quite sure how all such packages would depend on each other, but it actually might enforce prefixing client or server imports with the proper package name. So to access global Redux state you would have to type client/state/action-types, but to access local state you would be able to type state/action-types. I assume we might got rid of this NODE_PATH configuration at all with Lerna.

[coderkevin]

If we pick the same name for extension as the existing subfolder in client/ we can have a very similar collision.

I find this very unlikely, since the name of extensions is the name of the plugin they support at this moment. In fact, we could even add that to the linter process to enforce it doesn't happen.

Here's what we've got so far:

hello-dolly
sensei
woocommerce
wp-job-manager
wp-super-cache

Those are nowhere close to any normal subdirectory under client and I don't see collisions as very likely in the future.

[stale]

This issue has been marked as stale and will be closed in seven days. This happened because:

• It has been inactive in the past 9 months.
• It isn't a project or a milestone, and hasn’t been labeled `[Pri] Blocker`, `[Pri] High`, `[Status] Keep Open`, or `OSS Citizen`.

You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation.