search

jwplayer / ott-web-app

Reference implementation for JWP-powered apps
Apache License 2.0
70 stars 52 forks source link

Refactor: workspaces #404

Closed ChristiaanScheermeijer closed 8 months ago

ChristiaanScheermeijer commented 9 months ago

This PR couldn't be found... 😄

Description

This PR transforms the OTT Web App into a monorepo (workspace) to make the existing source code re-usable outside the web platform.

This process has been broken down into two steps. The first step is to set up the monorepo and separate the code into multiple packages. I've tried my best to prevent changing existing functionality. This will help us better overview all code changes in a separate PR.

Changes:

Unit testing:

For easy service/controller mocking, a utility has been added. This might change in the future due to issues when using it.concurrent, but it works for now when testing in series (test files are running parallel by default).

import React from 'react';
import AccountController from '@jwp/ott-common/src/stores/AccountController';
import { mockService } from '@jwp/ott-common/test/mockService';
import { DEFAULT_FEATURES } from '@jwp/ott-common/src/constants';

import { renderWithRouter } from '../../../test/utils';

import AccountModal from './AccountModal';

describe('<AccountModal>', () => {

  test('should call getUser when rendering', () => {
    // mock AccountController
    const { getAccount } = mockService(AccountController, {
      getFeatures: vi.fn().mockReturnValue(DEFAULT_FEATURES),
      getAccount: vi.fn(),
    });

    renderWithRouter(<AccountModal />);

    // expectations can be made on service/controller calls
    expect(getAccount).toHaveBeenCalledTimes(1);

    // implementations can be mocked
    getAccount.mockResolvedValue({ id: 123456, name: 'John Doe' });
  });
});

Updates

TODO:

github-actions[bot] commented 9 months ago

Visit the preview URL for this PR (updated for commit bae91fe):

https://ottwebapp--pr404-feat-ott-workspaces-6ywaigqx.web.app

(expires Sun, 14 Jan 2024 14:35:49 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

dbudzins commented 9 months ago

So I'm just finally getting deep into reviewing this, and I'm a bit confused/skeptical at the workspace implementations.

Some packages feel like they're being used as packages for the sake of packages, but that there are probably equally as good other ways to achieve the same thing (linting, i18n.) These tools are usually made to cascade rules from a common root, so what is the reason for abstracting here? My preference would be not to over complicate these if we don't need to.

The other packages make sense I guess, but they seem more like buckets than packages per-se, because they don't really have single responsibilities the way a typical library or sdk would. I think it would be awkward to deploy these as compiled versions to npm for example. I guess the reason for doing this as workspaces instead of just relative paths and aliases is for the sake of dependency unification, right? Or are there other major benefits that I'm missing?

It also seems a bit odd that none of your packages have build scripts and you're referencing the code from them including /src/ in the path. Shouldn't they be treated as compiled dependencies? Is there something setup funny?

My other major suggestion is that we try to take this in a few steps (they can all be in rapid succession, but keeping them as separate commits will hopefully make the merge conflict pain just a little bit less.) Basically I would try something like this:

  1. Move /src, /public, /test-e2e, /ini, etc. into /platforms/web
  2. Introduce yarn workspaces with just the existing code
  3. Setup empty packages for each of the packages
  4. Move common
  5. Move hooks
  6. Move ui

What I'm hoping is that steps 1, 4, 5, and 6 are essentially just file renames / moves in git. In fact it might even make it smoother if you create a path alias for each of the packages and first move the code to a top level folder with that alias within the current /src folder in 1 commit and then move to yarn packages in a 2nd commit. Maybe that's overthinking things, but it seems like it might be easy to fix a lot of conflicts in the first step with find and replace and then the second step would just be replacing # aliases with @ references.

ChristiaanScheermeijer commented 9 months ago

Thanks for reviewing this @dbudzins!

Some packages feel like they're being used as packages for the sake of packages, but that there are probably equally as good other ways to achieve the same thing (linting, i18n.) These tools are usually made to cascade rules from a common root, so what is the reason for abstracting here? My preference would be not to over complicate these if we don't need to.

You're correct, but there are a few reasons why I chose to create packages for linting configuration;

For monorepos, this is a typical pattern. We should consider each package to be an individual codebase with unique dependencies. For linting, we don't want to enable Stylelint for React Native platforms or Eslint with React for LightningJS platforms. By creating packages, we can choose what linting we want to enable from the package/platform level instead of modifying files in the root. This should prevent merge conflicts in syncs later on.

The only "shared" configuration is the base TSConfig. But this is extended from each package/platform and changed accordingly.

The goal for the i18n package was to have a single source of truth for translation files (JSON). But I didn't have enough time to figure out how to make this work with the i18next backend. We can remove this package and add it once we figure out how.

The other packages make sense I guess, but they seem more like buckets than packages per-se, because they don't really have single responsibilities the way a typical library or sdk would. I think it would be awkward to deploy these as compiled versions to npm for example. I guess the reason for doing this as workspaces instead of just relative paths and aliases is for the sake of dependency unification, right? Or are there other major benefits that I'm missing?

It also seems a bit odd that none of your packages have build scripts and you're referencing the code from them including /src/ in the path. Shouldn't they be treated as compiled dependencies? Is there something setup funny?

You're also correct here, but the reason for this is that we didn't do the refactoring yet. After the refactor, each package could be considered a standalone library/SDK. We didn't plan to publish these packages to NPM, but we could do so. Although not in the current state.

Choosing workspaces with package separation is mostly for dependency orchestration and having separate workspaces for different platforms/frameworks. Without this, we have mixed platform/framework dependencies in the package.json. Besides that, we don't need aliases indeed and we have the option to publish these packages to NPM more easily in the future.

The only part missing (to consider each package standalone) is the build and compile step for each package. I did this on purpose for the sake of simplicity. Otherwise, we would need to run multiple dev servers or watchers to compile each time we change something in one of the packages. Currently, the platforms registered in this repo are the only consumers for the packages. This is not a super common pattern (so far I know), but this is how we've done multi-platform monorepos for years at our company.

My other major suggestion is that we try to take this in a few steps (they can all be in rapid succession, but keeping them as separate commits will hopefully make the merge conflict pain just a little bit less.) Basically I would try something like this:

If the only thing changing is the addition of the platforms/web and commit structure, it should be doable. The added benefit of the platforms/web change is that we don't need to register each platform explicitly in the root package.json and vitest.workspace.ts. We can use glob patterns like those used for the packages folder.

Do you also propose to not squash this PR into a single commit for easier rebasing for developers using this repo?

dbudzins commented 9 months ago

@ChristiaanScheermeijer thanks for the thorough reply. A few comments below. No showstoppers, but want to get clarity and alignment before we lock in some decisions.

For linting, we don't want to enable Stylelint for React Native platforms or Eslint with React for LightningJS platforms.

I haven't done much with non web js development, but why would we not want to lint these platforms? As long as we have js/ts files, why not?

By creating packages, we can choose what linting we want to enable from the package/platform level instead of modifying files in the root.

Can't we do the same thing with a .eslintrc file in each directory where we want to change rules?

each package could be considered a standalone library/SDK

For this reason, I'm a bit hesitant to move things in big buckets by type (i.e. hooks, ui, etc.), especially if we end up moving a lot of files later to form more cohesive, logical groupings. (I'm thinking of packages organized by business logic, like epg, subscription, etc.) Do you need to move everything that you are moving for the work that depends on this downstream? If not maybe it makes sense to be more targeted about the moves?

The only part missing (to consider each package standalone) is the build and compile step for each package.

Is this really that much more code churn? I think it will actually simplify things a bit, because won't it allow package references to drop the src/ path and actually reference the output of each other? If we postpone that until later it will mean another huge PR with path changes, no?

this is how we've done multi-platform monorepos for years at our company

OK, I'm willing to suspend judgement for a while to see how it comes together in the 'final' form

we don't need to register each platform explicitly in the root package.json and vitest.workspace.ts. We can use glob patterns like those used for the packages folder.

Sounds good

Do you also propose to not squash this PR into a single commit for easier rebasing for developers using this repo?

I want to make merging as painless as we can. What do you think is the best approach here?

ChristiaanScheermeijer commented 9 months ago

@ChristiaanScheermeijer thanks for the thorough reply. A few comments below. No showstoppers, but want to get clarity and alignment before we lock in some decisions.

No problem! 😄

I haven't done much with non web js development, but why would we not want to lint these platforms? As long as we have js/ts files, why not?

Because these platforms will require different configurations and dependencies, it makes more sense to configure them from the package/platform respectively. This will also prevent merge conflicts when syncing because you won't need to worry about updates in a root file (e.g. <project>/.eslintrc).

React Native will never use SCSS (Stylelint) and LightningJS will never need React.

Can't we do the same thing with a .eslintrc file in each directory where we want to change rules?

We could, but that would mean all frameworks/platforms will be combined in a single eslint file, most likely resulting in merge conflicts. The packages provided by the repo can be used for React and TS combinations, but when adding a different platform, you can use a different config.

For this reason, I'm a bit hesitant to move things in big buckets by type (i.e. hooks, ui, etc.), especially if we end up moving a lot of files later to form more cohesive, logical groupings. (I'm thinking of packages organized by business logic, like epg, subscription, etc.) Do you need to move everything that you are moving for the work that depends on this downstream? If not maybe it makes sense to be more targeted about the moves?

I've tried combining as much as possible with framework separation in mind. E.g.

I don't think grouping on feature level won't work well in this case, because we want to be able to re-use most hooks and business logic (services, stores and controllers) for React and React Native apps. When introducing feature buckets, we still need to separate the UI from logic to make it usable for other frameworks.

The other downside is that you'll need a different eslint/tsconfig file for different parts in the same bucket.

Is this really that much more code churn? I think it will actually simplify things a bit, because won't it allow package references to drop the src/ path and actually reference the output of each other? If we postpone that until later it will mean another huge PR with path changes, no?

We can look into this if you think this improves the current structure. But I'm afraid that this is only possible after the refactoring because too many dependencies (e.g. React/window usage) in common now make bundling the packages difficult. This might be a good test after the refactoring because this should be possible then.

I do like it when we can drop the src part in the imports and make the monorepo more "standard". Do you still like the separate imports instead of unifying them with the use of index.ts(x) files?

For example;

import { Button, Dialog } from '@jwp/ott-ui-react/components';
import { AccountService, FavoriteService } from '@jwp/ott-common/services';

// VS

import Button from '@jwp/ott-ui-react/components/Button/Button';
import Dialog from '@jwp/ott-ui-react/components/Dialog/Dialog';
import AccountService from '@jwp/ott-common/services/AccountService';
import FavoriteService from '@jwp/ott-common/services/FavoriteService';

I want to make merging as painless as we can. What do you think is the best approach here?

I agree, but I'm afraid this will become difficult either way.

The current strategy is to help users make customizations easier without modifying the existing codebase. In other words, I think merging this back into a customized OTT Web App shouldn't be the goal. Instead, we want users to leverage the options provided by the restructure to make their own OTT platform and make it easier to keep it in sync once upgraded to this version.

dbudzins commented 9 months ago

OK, I guess let's proceed as is for the most part. We will learn a lot when the first new platform is introduced. We can also see what sort of feedback we get from customers about merging.

As for index files, I'm a bit ambivalent. If there are some cases where it cleans things up a bunch, feel free, but they should always be limited to simply grouping exports, no logic.

ChristiaanScheermeijer commented 8 months ago

@dbudzins @AntonLantukh I've rebased this PR with the develop branch and moved the web platform to a new platforms directory. All configurations are updated to use the new platforms directory.

What do you think?