Site-kit-wp: Use only project-relative JS imports for first-party code dependencies

[felixarntz]

Feature Description

We currently use a couple of aliases in our JS code that resolves to our own code. We should instead use project-relative imports only for those.

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• There should no longer be aliases in Webpack config for Site Kit's own JS code.
• All import statements for internal dependencies should be modified to become relative, except if they are importing from an entrypoint / public API, such as (this may not be an exclusive list):
  
  
  
  • googlesitekit-api
  
  
  • googlesitekit-data
  
  
  • googlesitekit-modules
  
  
  • googlesitekit-widgets
  
  
• All import statements changed should be correctly categorized as "Internal dependencies".
• Exceptions for the Eslint rule @wordpress/dependency-group should be removed/fixed.

Implementation Brief

• Remove all resolve aliases in favor of file-relative imports for the found here with the exception of the api-fetch related ones.

QA Brief

• Test the plugin ensuring it works as expected before the refactor.

Changelog entry

• N/A

[tofumatt]

IB written, but a few questions about it before it's finalised. Neither answer will significantly affect the time to complete the PR, so the estimate should remain either way 😄

(Note that the estimate is a bit higher here because of the likelihood of a fair bit of manual intervention in re-ordering import statements throughout files where external and internal ones are mixed up. I erred on the side of more time, but hopefully it doesn't take that long and/or eslint can fix a fair few automatically, but I didn't want to assume 😃.)

[felixarntz]

IB ✅

@tofumatt
Let's keep your second question out of this one for now. I'm onboard with it, but it should be a separate issue. The renaming of assets to src should be well-timed with other work that may be going on, and such a PR shouldn't include more than the rename, to make fixing other ongoing PRs as straightforward as possible.

Introducing asset-level imports sounds good to me, particularly because of your second question. That would mean that when we change assets to src, we wouldn't need to adjust any of the import statements, but just the definition in the config right?

[tofumatt]

Let's keep your second question out of this one for now. I'm onboard with it, but it should be a separate issue. The renaming of assets to src should be well-timed with other work that may be going on, and such a PR shouldn't include more than the rename, to make fixing other ongoing PRs as straightforward as possible.

Totally! That makes sense; I figured I'd bring it up, but sounds good—we can do that during a "quiet time" with PRs and such 😄

Introducing asset-level imports sounds good to me, particularly because of your second question. That would mean that when we change assets to src, we wouldn't need to adjust any of the import statements, but just the definition in the config right?

That's right, and a great point 🙂We can do the rename and just change one line in the webpack config (assets -> src). Sounds good, I've added the asset-level imports to the IB from the questions. 👍

[aaemnnosttv]

Hey @felixarntz, - @tofumatt and I discussed this last week and we came to the conclusion that this issue is (or at least should be) primarily about eliminating the use of aliases _altogether_. Aliases would be nice if there was better support for them in our tooling, but they end up adding more burden than benefit.

Tooling doesn't understand aliases and so not only do we need to maintain configuration between our main webpack config, but also for storybook and jest. Another big reason to get rid of aliases is that IDEs like VScode, etc can't resolve them, so we lose valuable time-saving features, like auto-imports and auto-updating imports when a file is moved or renamed, not to mention simple everyday things like clicking on import to navigate to the file, or jumping to the definition of an imported entity.

The bottom line is that using project-relative imports are essentially aliases too, albeit more explicit ones, they're essentially the same.

After discussing this, Matt and I came to the agreement that we should do away with _all_ aliases, and use file/module-relative imports for everything that doesn't come from node_modules. This is more consistent with WP core/Gutenberg as well, which does not use any aliases.

If this is okay with you, this change shouldn't really impact the effort needed for this issue, although it would need updated AC+IB, it's more about what the existing aliases should be changed _to_.

I'll move this back to AC for the time being just so it doesn't inadvertently get started with the current definition, but it shouldn't be a setback.

(@tofumatt feel free to elaborate on anything I may have missed or potentially misrepresented here, if anything 😄)

[tofumatt]

Nothing to add; that's the perfect write-up of the situation. 👍🏻

Essentially, I had always liked—and advocated for—project-relative imports before I started using an editor (VSCode) with better tooling. But the power offered by using file-relative imports greatly outweighs the _slightly-prettier_ and more _human-readable_ import blocks at the top of the file.

We should move to this 😄

[felixarntz]

@tofumatt @aaemnnosttv I agree with these changes, however there is one caveat that we'll have to account for, which is that we sometimes need to import from our own "public packages". We do currently rewrite such imports via Webpack externals and need to continue to do so - that itself is probably not an issue. However, we need to differentiate because sometimes we'll also directly import code from these packages (when that code is part of the module, but not publicly exposed). Would the differentiation here maybe be assets/js/googlesitekit-{module} (public) vs assets/js/googlesitekit/{module} (private)?

[tofumatt]

For public imports of things like googlesitekit-data we can continue to use the aliases at first, but after this main issue is complete I can see if webpack will be smart enough to treat any import that resolves to /assets/js/googlesitekit/data as the external.

Basically: for now we should leave imports of googlesitekit-* alone when they are like this: https://github.com/google/site-kit-wp/blob/bc8e51999da45896969e6a88a901f33fc4e9a478/assets/js/googlesitekit/data/create-settings-store.js#L29
Anything else can be safely changed 😄

[felixarntz]

@tofumatt I've modified the ACs accordingly, so this is ready for IB now. Let's move it forward.

[felixarntz]

@ryanwelcher IB on a high level ✅

Let's make sure to not rewrite the specific ones listed in the ACs, and also adjust the eslint statements on "... dependencies" accordingly.

[tofumatt]

Code reviewed, tested locally, and works great!

Just needs either @felixarntz or @aaemnnosttv to merge, awesome work!

[tofumatt]

QA ✅All webpack aliases are out and things still work, yay! 👍

Shouldn't require further QA—if nothing has broken thus far I'd say this one's good, as webpack itself would have error'd during build referencing any bad paths, so moving to approval.