plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
452 stars 612 forks source link

@plone/types dependency conflict with react-intl's React peer dependency #6115

Open JeffersonBledsoe opened 3 months ago

JeffersonBledsoe commented 3 months ago

react-intl@3.8.0 has a peer dependency of React 16

We can use an override for now in package.json, but it's not ideal:

"overrides": {
    "react-intl": {
      "react": "^16.8.0 || ^17.0.0 || ^18.0.0"
    }
  }

It would be REALLY nice if we could just avoid bringing in anything React-related at all for this dependency and just have it provide types for services and raw content

npm i -D @plone/types
npm error code ERESOLVE
npm error ERESOLVE unable to resolve dependency tree
npm error
npm error While resolving:my-package@0.0.1
npm error Found: react@18.3.1
npm error node_modules/react
npm error   peer react@"^16.8.0 || ^17.0.0 || ^18.0.0" from @plone/types@1.0.0-alpha.16
npm error   node_modules/@plone/types
npm error     dev @plone/types@"*" from the root project
npm error   peer react@"^18.3.1" from react-dom@18.3.1
npm error   node_modules/react-dom
npm error     peerOptional react-dom@"^16.8.0 || ^17.0.0 || ^18.0.0" from @plone/types@1.0.0-alpha.16
npm error     node_modules/@plone/types
npm error       dev @plone/types@"*" from the root project
npm error
npm error Could not resolve dependency:
npm error peer react@"^16.3.0" from react-intl@3.8.0
npm error node_modules/react-intl
npm error   peer react-intl@"3.8.0" from @plone/types@1.0.0-alpha.16
npm error   node_modules/@plone/types
npm error     dev @plone/types@"*" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.
ichim-david commented 3 months ago

@JeffersonBledsoe @sneridagh take into consideration that I have close to 0 experience with typescript. I did ask claude sonnet 3.5 if it's a pattern to replace type definitions with simplified definitions. It gave me this reply:

Yes, you can define the intl property for the BlockViewProps interface without directly depending on react-intl. Here's how you can do it:

First, create a simplified version of the IntlShape interface in your @plone/types package. This will contain only the essential properties and methods that you need for your use case.

Use this simplified interface instead of the one from react-intl.

Here's an example of how you could define this:

// In @plone/types/src/intl/index.d.ts

export interface SimplifiedIntlShape {
  formatMessage: (descriptor: { id: string; defaultMessage?: string }, values?: Record<string, any>) => string;
  formatDate: (value: Date | number, options?: Intl.DateTimeFormatOptions) => string;
  formatTime: (value: Date | number, options?: Intl.DateTimeFormatOptions) => string;
  formatRelative: (value: Date | number, options?: Intl.RelativeTimeFormatOptions) => string;
  formatNumber: (value: number, options?: Intl.NumberFormatOptions) => string;
  formatPlural: (value: number, options?: Intl.PluralRulesOptions) => string;
  // Add any other methods you need
}
// In @plone/types/src/blocks/index.d.ts

import { SimplifiedIntlShape } from '../intl';

export interface BlockViewProps {
  // ... other properties
  intl: SimplifiedIntlShape;
  // ... rest of the properties
}

This approach has several advantages:

It removes the direct dependency on react-intl from @plone/types. It provides type checking for the most commonly used intl methods. It's flexible enough to be used with different internationalization libraries that provide similar methods. When using this in your actual Volto components, you can still use the full IntlShape from react-intl, as it will be compatible with this simplified version.

If you need to add more methods or properties to the SimplifiedIntlShape interface in the future, you can do so without affecting the rest of your codebase. END chat response.

With how many functions we call from intl it seems like an easy thing for us todo and provide simplified types. Performing a search gave me some replies that this is a common pattern todo in case you need it. I don't know what Victor thinks about it though so I am leaving this comment to hear his opinion on such a proposal in this case knowing that it has that old react version as peer dependency.

JeffersonBledsoe commented 3 months ago

@ichim-david I'm not sure how I feel about copying the type definitions. While would reduce the dependencies, it is ultimately masking the fact that we are dependent on react-intl. We would need to come up with a way to keep the types in-sync and it could potentially make things harder to debug. That said, it's a good practical approach to a 'good enough' system for the reasons you outlined!

ichim-david commented 3 months ago

"It would be REALLY nice if we could just avoid bringing in anything React-related at all for this dependency and just have it provide types for services and raw content". @JeffersonBledsoe we are dependent on react-intl due to it's usage in the props. How would the types look like without then any react related types in plone/types?

How would it work? I feel like we need a bit of an actual suggestion with what/how to replace them.

In a way we have this requirement of plone/components https://github.com/plone/volto/tree/main/packages/components

Questions:

  1. Do we need for types to also be dumb and agnostic?
  2. Is there a need for a base types and extended?

Again these are probably some questions and answers to be given as there is more involvement in the community for using the type package and would need some answers from people with way more experience than I have. Less complicated is probably what is desired but I don't know what that means for the end user and the maker of the types.

JeffersonBledsoe commented 3 months ago

How would it work? I feel like we need a bit of an actual suggestion with what/how to replace them.

I think what I would like to see is the ability to get types for API responses (be that REST API or @plone/client, as they should match) without being tied to any particular framework or implementation. This way, we are able to use @plone/types and @plone/client however we see fit (for example, I'm trying to use it in a test Astro project).

Then for the next steps, I'd like to see the same thing but for Volto configuration, such as the site settings, block schemas, etc. This second stage is much harder than the first (as currently types, schema and rendering of the block are all loosley intertwined) with potentially a smaller number of developers who would benefit from the change, so I don't think is worth exploring yet.

@sneridagh Thoughts?

sneridagh commented 3 months ago

@JeffersonBledsoe @ichim-david I am happy that you are trying to work with the packages, because you managed to feel the kind of challenge that we are facing, and we will face in the times to come. Yet, this is only one of many. I solved it in the best way that I could/know and applying a bit of pragmatism too to move forward.

The fact is, that Volto uses it extensively, and some artifacts expect intl as a prop. And as facts, they are undeniable, and we need to model it in our Volto typings.

@JeffersonBledsoe I'd like to know exactly the source of your problem too, and what is the solution that you applied. So, the problem is that npm is complaining about react? I can tell from the solution you are applying in the resolutions.

npm is very unflexible, and errors if something is wrong. Aside that is the worst package manager nowadays, I would never use it for any kind of project. Also, using --legacy-peer-deps then it will work. Why are you using it? In the Volto monorepo we don't have this issue, and both Volto and the PoCs are using React 18. But of course, using another package manager does not solve the real problem.

I agree we should do better, and reduce the peerDepencencies specially on the foundamental packages (@plone/registry, @plone/client, @plone/components and @plone/types). I'd go for the proposal that @ichim-david hinted. So we "mock" the best possible the intl object, ideally, matching the one in react-intl@3.8.0, the one we use in Volto. Having a "mock" is like, "ok I'm not using the real thing, so in order to spare it, I vendor it locally". Which is ok, but the real thing is that Volto typings really depend on react-intl@3.8.0, and that's undeniable, as I stated it before...

If at some point, we are using other react-intl version in Volto, then we will update it.

If in other tools we have to use it also, we can adapt to the i18n tool of choice by overriding the type in the corresponding project with the current intl shape (or the one that we made up in there).

These are all conjectures, of course, because if you are using another i18n tool, I have no idea to what challenges we will be facing then, so could be that one had to write a complete different set of typings for that artifact...

Whatever solution we adopt, we should test it throughfully, and assess that the typings match (eg. as in using them using TS in Volto, and the "real" react-intl typings do not complain).

However, this is the exact use case for which peerDependencies are for, so you declare them, and use it if needed or not. yeah it sucks to have in development a package that you don't use.

Next Volto team meeting subject to discuss about?

JeffersonBledsoe commented 3 months ago

I'd like to know exactly the source of your problem too, and what is the solution that you applied. So, the problem is that npm is complaining about react? I can tell from the solution you are applying in the resolutions.

@sneridagh You can directly recreate the problem fairly easily by trying to install @plone/types in a non-react framework. E.g. with astro: npm create astro@latest -- --yes && cd helpless-hubble && npm install -D @plone/types

This fails with the exception linked in the issue body. I can get around the issue by using the override mentioned (to get around the old version of react-intl we have), but I still end up installing react and adding it to my lockfile, even though I'm not using it.

In this case, I'm trying to use @plone/types to add types to REST API calls:

import type { NavigationResponse } from '@plone/types';
const response = await fetch(
  'http://localhost:8080/Plone/++api++/@navigation?expand.navigation.depth=2',
);
const result: NavigationResponse = await response.json();

In this scenario, where I'm not using React, having to install react is a bit wasteful (even if I can just ignore it being there, it's still a massive clog up of the lockfile and waste of bandwith). I ideally would want @plone/types to be 'vanilla typescript', and then have some way of importing the react dependencies (e.g. @plone/types/react) separately

However, this is the exact use case for which peerDependencies are for, so you declare them, and use it if needed or not. yeah it sucks to have in development a package that you don't use.

I have to disagree with this. peerDependencies are for declaring the version compatibility of a package. optionalDependencies would be more appropriate for something that you don't need to install to get it to work. But like you already mentioned, react-intl is required by @plone/types due to the UI and intl definitions within that package. I don't really have an optimal solution here and would definitely be up a discussion and/ or using the mock typessuggestion proposed by @ichim-david!