Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] - Accommodating code to render for apps inside dotcom-rendering/ #7174

Closed
georgeblahblah opened this issue Feb 10, 2023 · 7 comments
Closed

Comments

@georgeblahblah
Copy link
Contributor

georgeblahblah commented Feb 10, 2023

[RFC] - Accommodating code for apps rendering inside dotcom-rendering/

🎉 The DC-A-R project is starting! DCR will render articles for an additional platform: the mobile apps. This is in addition to the existing platforms, amp and web.

To accommodate this new platform, we need somewhere to put code specific to rendering for apps. We might also need to move code which is shared between platforms, especially as it's likely we will be able to use many of DCR's web components for apps as well.

Before thinking about where to put new code or move existing code to, there are some questions to consider (and possibly more not listed here!):

  1. How can we make sure the DCAR project does not make the DCR repository much harder to navigate or work with?
  2. How can we make it hard to forget that some changes affect multiple platforms, e.g web and apps?
  3. How can we create a structure which allows us to leverage DCR's capabilities when developing for apps?

A starting point for discussion

In the "apps rendering in DCR" spike, we created an app sdirectory to sit alongside web/ and amp/:

src/
    apps/
    web/
    amp/
    ...

This helped keep app-specific code separate, but led to apps layouts importing a lot from web's components. This coupling wasn't very clear when working from the web/ directory, but it didn't create any significant problems during the spike.

The main idea of this starting point: create an apps/ directory for apps-specific code and an editions/ directory, and move things which are shared by multiple platforms into a src/common/ directory. This could give a structure that looks something like:

src/
    common/
        components/
        client/
        lib/
        server/
        ...
    amp/
        components/
        lib/
        pages/
        ...
    apps/
        client/
        components/
        layouts/
        lib/
        server/
        ...
    editions/
        client/
        components/
        ...
    web/
        client/
        components/
        ...

This is just a starting point for discussion. I'm curious to hear any thoughts, alternatives or concerns!

Additional questions

  • How do we want things to look in Storybook?
  • Alternatively, could we rename web/ to something, and nest app/ in there?
@JamieB-gu
Copy link
Contributor

Thanks for writing this up @georgeblahblah! I suppose a key consideration for this will be how much we want to refactor the existing DCR codebase? Do we want to keep changes limited for now to minimise the impact on others working on the project? Or are we happy to make a more drastic change? I would be interested to hear from the rest of the team on this.

This could give a structure that looks something like:

Assuming the file structure above, I have a couple of small suggestions:

  • Could we rename app to apps?
  • Could we add a directory for editions?

I also have an alternative suggestion whereby we flatten the file structure and assume common code by default, with the occasional sub-directory for product-specific code:

src/
    components/
        apps/
        web/
        Headline.tsx
        Standfirst.tsx
        ...
    server/
    client/
    contributions.ts
    decidePalette.ts
    ...

This might prove useful to encourage consistency and common code, as opposed to siloing by product. That said, this would be a more significant change to the codebase, and it may be too early to consider something this major, as discussed above.

There's another possibility for organising components, which I think we may have discussed during the spike?

components/
    Metadata/
        index.tsx
        apps.tsx
        web.tsx

I would be keen to hear people's views on any of the above. Ultimately, I think we want whichever makes the project easiest to reason about and maintain for most people.

How do we want things to look in Storybook?

I think the storybook question mirrors the file structure question? We could have common, apps and web folders, as in your original example. We could have most components at the root, with small folders of apps, web, editions and amp components. Or we could have all components at the root, with variants under some of them for apps, web, editions etc.

@georgeblahblah
Copy link
Contributor Author

@JamieB-gu thanks so much, these are all really great points. I have renamed app => apps, and added editions in the examples.

I also like your alternative suggestions - especially to consider having an even flatter structure and assume modules are common by default.

I suppose a key consideration for this will be how much we want to refactor the existing DCR codebase?

yes, that's something we should definitely keep in mind. Depending on the preferred option, it might be possible to move things as we touch them rather than all at once - as long as that doesn't create too much confusion itself. I think this is highly dependent on the structure we choose.

Do we want to keep changes limited for now to minimise the impact on others working on the project? Or are we happy to make a more drastic change?

I think this is a question for the wider team. I'm generally in favour of minimising initial impact. With that in mind, we could also follow the approach in the spike, whereby we imported from web/components. We can then evaluate which structures make the most sense, or solve the most problems.

@OllysCoding
Copy link
Contributor

This is great @georgeblahblah :)

I think this is a really interesting question, which is not helped by the fact that the current DCR structure is poorly defined. I think for this reason, we should consider this an opportunity to bring clarity to directory structure where there might not have been before.

I also have an alternative suggestion whereby we flatten the file structure and assume common code by default, with the occasional sub-directory for product-specific code:

I'm actually a big fan of this solution @JamieB-gu, I think this avoids the risk of over-abstraction. I think this would also be a good opportunity to take a look at the AMP directory (which has fallen behind web in some ways), and try and move it to help keep it more up-to-date with the rest of the project.

I think we'd definitely need product specific directories for things like server/, client/ and lib/, e.g:

src/
    components/
        apps/
        web/
        amp/  
        Headline.tsx
        Standfirst.tsx
        ...
    server/ # Could this be one dir? Maybe with the right file names, but I'm not sure what that would be
        amp/
            lib/ # Is this needed? Currently amp has its own lib dir for server, there's a root server/lib, but not a web/server/lib
        web/ 
            lib/
        apps/
            lib/
    client/
        # Can we get away without platform dirs here? No client code for AMP, will apps/ need to differ from web?
    lib/
         # Might we need /amp /apps & /web here? 
         contributions.ts
         decidePalette.ts
    types/
         # Do we want to keep a types dir? We've been gradually moving away from using index.d.ts for types that don't need to be global.
    ...

One question I'd have about having the above structure would be whether it'd interfere with @jamesgorrie future ideas around separating DCR by purpose (e.g articles, fronts, etc). My initial thought it no, and that any form of better definition around file structure will enable us to make that kind of change better in the future, but perhaps the initial suggestion by @georgeblahblah would be better for this?

@Georges-GNM
Copy link
Contributor

Georges-GNM commented Feb 14, 2023

Just my 2 cents, I like the common code approach as well - the way I see it, it feels like a continuation of common-rendering in a way; which made sense to me as an approach, but I guess ended up not being as beneficial as hoped with AR and DCR being too separate projects until now.

I'd also suggest we can still separate by purpose further down the tree as needed, something like this I think would make sense and probably be helpful in finding things swiftly:

src/
    components/
        apps/
        web/
           fronts/
              Cardheadline.tsx
              Frontcard.tsx
           articles/
              ArticleBody.tsx
              TableOfContents.tsx
        amp/  
        Headline.tsx
        Standfirst.tsx
        ...

@georgeblahblah
Copy link
Contributor Author

georgeblahblah commented Feb 15, 2023

Thanks everyone for your fantastic suggestions so far.

To make sure we can get started sooner rather than later, we will take the approach of "least changes" initially. We will leave most existing code as-is, and create an apps/ subdirectory of src/:

src/
  apps/
  web/
  ...

This means apps/ will import a lot from web/. There are some key benefits to this approach, although it is messier initially:

  • we can start soon, having a place to put apps-specific code
  • when we do come to refactor the folder structure, it will be clear if there are any additional inputs to the decision in addition to what we have considered here
  • as we start refactoring, it will be clear whether it is solving the problem or introducing other difficulties

We will look back to the suggestions here when we revisit the folder structure again, in a few sprints time.

@sndrs
Copy link
Member

sndrs commented Mar 2, 2023

Apologies for butting in, but I missed this earlier and I love this kind of stuff :) do ignore this if its not helpful...

What do you think about something along these lines?

applications/
├── article.immersive.client.ts
├── article.immersive.server.ts
├── article.standard.client.ts
├── article.standard.server.ts
├── front.client.ts
├── front.server.ts
libs/
├── components/
│   ├── Button.tsx
│   ├── Cmp.apps.tsx
│   ├── Cmp.dotcom.tsx
│   └── Header.tsx
└── layouts/
    ├── Article.immersive.tsx
    ├── Article.standard.apps.tsx
    └── Article.standard.dotcom.tsx

Here, Button.tsx, Header.tsx and Article.immersive.tsx are either used in both apps and dotcom, or only one of them (e.g. Header.tsx).

I think it allows for common components where they're simple enough to not need alternate versions, and for alternate versions where necessary, while making it immediately clear to the first-time browser how to find things and what's happening in each case.

The other thing this heavily borrows from is Nx's mental model of an applications/libs distinction. In this way of thinking, you put as much business logic as practical into applications, and things in libs should be as library-like as possible.

That is, things in applications handle business logic (e.g. different data models) and compose things from libs, while things in libs do not really know a lot about the things that compose them.

@jamesgorrie
Copy link
Contributor

we flatten the file structure and assume common code by default

This is great as it creates fewer categorisations which I don't feel we've defined as yet (see github.com//issues/7256 for example).

It also addressed something we should ensure is the case, if things are coupled, they are explicitly so, maybe reframing your consideration @georgeblahblah

How can we make it hard to forget that some changes affect multiple platforms

=>

How can we make it evident that a change will affect 1 or multiple rendering targets


Do we want to keep changes limited for now to minimise the impact on others working on the project

If we have a larger scale refactor it feels considerate if it:

  • does not disrupt others work
  • does not allow for two+ patterns of doing things, creating cognitive overload (the refactor should be a complete refactor or have an iterative plan)
  • does not change repeatedly needing someone to repeatedly relearn a new pattern
  • should try not to have a tonne of conflicts on people's current work

These are in line with - "I want to deliver a thing, not learn your platform" and from the perspective of people delivering features on the platform.

To this end, and that we're still exploring, @JamieB-gu's suggestion offers an easy path forward.

We could make it lighter touch to allow for some validation and change on the way. If we maintain ./src/web as the root and just imagine it is ./src, so that the refactor should be more seamless.

e.g.

  1. create ./src/web/server/apps/endpoint.ts
  2. create ./src/web/components/apps/Layout.ts
  3. Start rendering out from there
  4. 🏁 Checkpoint to see if it's working
    1. If so - Move web rendering target into it's own directories .src/web/web/
      1. remove ./web directory
    2. If not, find another approach, none of the code will be lost, just relocated

the above structure would be whether it'd interfere with @jamesgorrie future ideas around separating DCR by purpose

💯 agree, I don't think this interferes with that at all.

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

6 participants