mattmilburn / strapi-plugin-preview-button

A plugin for Strapi CMS that adds a preview button and live view button to the content manager edit view.
MIT License
106 stars 38 forks source link

Switch from `useCMEditViewDataManager()` to `useDocument()` #140

Open jackbuehner opened 2 months ago

jackbuehner commented 2 months ago

This PR finishes replacing usage of @strapi/helper-plugin. (@strapi/helper-plugin is deprecated and is not compatible with Strapi 5.) It builds on builds on #133. useCMEditViewDataManager() from @strapi/helper-plugin has no direct replacement, but the part used by this plugin is replaced by useDocument() from @strapi/admin/strapi-admin.

This PR also switches the <Button /> component usage to <LinkButton />. They look the same, but they allow right clicking to copy links and middle clicking to open in a new tab. If you click them, they behave exactly as the always have; event.preventDefault() is called and the regular logic takes over.

mattmilburn commented 2 months ago

Hi @jackbuehner I really appreciate the work you put into this PR! 🎉 I hate to break it to you 💔 that I've already handled a lot of this upgrade before seeing your PR, and I converted everything into TypeScript so it presents quite a conflict with your PR.

However, would you be interested in still rebasing this branch with the latest changes in PR 133 and migrating the useCMEditViewDataManager usage in EditViewRightLinks admin component? Also, I wouldn't mind if you wanted to rebase everything and force push the changes to avoid some ugly commit history.

I wonder if that implementation is going to change before the final v5 release but either way, it would be nice because I have not worked with migrating that functionality in a plugin yet 😄 Plus I don't want to steal your Github cred and this would be a good opportunity to be listed as a contributor.

I'm happy to discuss other improvements that might be needed too.

jackbuehner commented 2 months ago

No worries! I like the move to TypeScript. I would have posted about contributing on the PR, but I was in the zone on updating everything and just decided to do it.

I'm happy to rebase with your latest changes. It probably can't do it today, but I can definitely do it before the end of the weekend, if that is okay.

They said there shouldn't be too many more big changes now that we are in the release candidate phase, so hopefully this is how it will be. But who knows – they have not yet fully documented the replacements for useCMEditViewDataManager.

jackbuehner commented 2 months ago

Are you okay with switching Button to LinkButton? I can also save it for a different PR if you want to keep things separated.

mattmilburn commented 2 months ago

Are you okay with switching Button to LinkButton? I can also save it for a different PR if you want to keep things separated.

Oh sure, LinkButton is fine 👍🏻

jackbuehner commented 2 months ago

Sorry it took a little longer (I got sick). I just did a hard reset to your version of the strapi5 branch. I then committed on top of that. It should make it so this PR can be cleanly merged into mattmilburn:strapi5 Let me know if you need me to change anything!

mattmilburn commented 2 months ago

@jackbuehner Here are some additional things I found after running yarn build to test your updates locally. Do you mind adding these into your PR? Sorry the first item was something I missed before.

  1. Add "styled-components": "^6.1.13" to both peerDependencies and devDependencies in package.json and run yarn.
  2. In ListViewColumnProps and AddPreviewColumnProps can you use uid: UID.ContentType | undefined instead of uid: string.
  3. In ListViewColumn, line 38, this hook can be simplified to usePreviewButton(layout.contentType.uid, data) to match your update to the hook param.
mattmilburn commented 2 months ago

@jackbuehner Also, have you tested your updates in a local Strapi app? When I configure the bare minimum config for a content type, I'm getting this error and I'm not sure what's causing it. Curious if you ran into this or not.

Screenshot 2024-09-18 at 2 30 05 PM
jackbuehner commented 2 months ago

@jackbuehner Also, have you tested your updates in a local Strapi app? When I configure the bare minimum config for a content type, I'm getting this error and I'm not sure what's causing it. Curious if you ran into this or not.

Screenshot 2024-09-18 at 2 30 05 PM

I did, but it was an earlier release candidate. Maybe something changed? I'll give it a look.

jackbuehner commented 2 months ago

The bundle has its own copy of @strapi/admin. This means that the useStrapiApp hook cannot find the StrapiApp context because they are separate. This was not a problem earlier in my PR because

You can verify that the bundled version is being used by adding console.log('WRONG CONTEXT') to the useContext function inside the createContext function in node_modules/@strapi/admin/dist/admin/Theme-frC82ceE.mjs:

function createContext(rootComponentName, defaultContext) {
  const Context = ContextSelector.createContext(defaultContext);
  const Provider = (props) => {
    const { children, ...context } = props;
    const value = React.useMemo(() => context, Object.values(context));
    return /* @__PURE__ */ jsx(Context.Provider, { value, children });
  };
  const useContext = (consumerName, selector) => ContextSelector.useContextSelector(Context, (ctx) => {
    console.log('WRONG CONTEXT')
    if (ctx)
      return selector(ctx);
    throw new Error(`\`${consumerName}\` must be used within \`${rootComponentName}\``);
  });
  Provider.displayName = rootComponentName + "Provider";
  return [Provider, useContext];
}

const [StrapiAppProvider, useStrapiApp] = createContext("StrapiApp");

It was not a problem earlier in my PR because I had used rollup and was not bundling .mjs files (which is what @strapi/admin was using). https://github.com/mattmilburn/strapi-plugin-preview-button/blob/43f794f46bb32ccd351f37493aee409c2030b1e2/rollup.config.mjs#L24C5-L26C8. I did this out of ignorance; I should have marked @strapi/admin as external.

strapi-plugin uses rollup behind the scenes. We need to set @strapi/admin as external with rollup, but I am not sure how we can do that with strapi-plugin build.

The code for build from strapi-plugin is at https://github.com/strapi/sdk-plugin/blob/c4702ad5c12fccc83530519a4c95814c09f17bdd/src/cli/commands/plugin/build.ts#L70C5-L75C8. The only options we can provide are the ones available via the command options at the bottom of the file.

Any ideas? @mattmilburn

mattmilburn commented 2 months ago

@jackbuehner Just an update - I'm reaching out to Strapi support on Discord. This is a popular plugin that even the Strapi team uses internally, so don't worry - we will get this figured out soon enough 🤞🏻

jackbuehner commented 1 month ago

Thanks @mattmilburn. My last thought was to use npx @strapi/sdk-plugin:init and try to add in bits of the code from this plugin until something breaks (and compare the package versions in package.json). I'll wait until you hear back from support on Discord.

jackbuehner commented 1 month ago

Might be related to https://github.com/strapi/strapi/issues/21151

danidoff commented 1 month ago

Hey guys. I think they fix it in strapi 5.0.3

mattmilburn commented 1 month ago

Unfortunately I'm still seeing the same error after updating to v5.0.3.

mattmilburn commented 1 month ago

@jackbuehner This is possibly fixed in v5.0.5 🤞🏻

EDIT: Actually, false alarm. The issue is still happening in v5.0.5 😞

SalahAdDin commented 1 month ago

Not able to migrate plugins to version 5 yet!

killthekitten commented 2 weeks ago

Same issue in v5.3.0

killthekitten commented 1 week ago

@jackbuehner @mattmilburn there was an update from the Strapi team in this comment. It appears that the bug is only present when the plugin depends on an RC release of Strapi. Could you try updating the plugin to the latest Strapi?

In my comment above I might have only bumped the main project's dependency.

mattmilburn commented 1 week ago

This is still a problem in v5.4.0. I've created an issue with Strapi to describe the problem in case anyone is interested.