Skip to content
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

Workaround for MC791596 #3142

Closed
wants to merge 1 commit into from

Conversation

jhholm
Copy link

@jhholm jhholm commented Sep 27, 2024

Category

  • Bug fix?
  • New feature?
  • New sample?
  • Documentation update?

Related Issues

Fixes #3095 + #3096 and related

What's in this Pull Request?

I changed the logic on updating the banner data without introducing breaking changes. It's not a 100% proper fix, but should work for most use cases.

Further information

I reverse engineered how the banner rework works. The old way of using a banner keeps the data in LayoutWebpartsContent field. The banner rework seems to keep that field as an empty array. Pages with the old banner format are migrated in browser when editing the page.

I created a simple wrapper which updates either the old way of using a banner or the new way of using a banner as a separate webpart.

I noticed that when modifying a client side page in browser with multiple banners, only the first banner is being updated so I mimicked that in the wrapper logic.

Tested the following use cases

  1. Create a clientside page with pnpjs and updating the page with pnpjs
    • This will keep the page with the old banner in LayoutWebpartsContent
    • You can normally edit the page in browser, which whill migrate to the new banner webpart
  2. Create a clientside page with pnpjs, update the page manually in browser
    • Editing in browser will update the page from the old banner format the new banner webpart
  3. Create a clientside page in browser with a banner
    • Will only edit the new banner webpart, and not save anything to LayoutWebpartsContent
  4. Modifying a page with multiple new banner webparts
    • Should only set the changes on the first banner on the page, as does updating the title of the page in a browser

These use cases will not work, and would probably require breaking changes

  1. Create a clientside page in browser and remove the new banner, update page with pnpjs
    • Logic will add the old banner
  2. Modifying a clientside page with pnpjs that has an old banner and a new banner
    • I don't see this as a viable real use case
    • Will actually edit the new banner and not the old banner

Tested with all the banner modifications (changing image, title, etc). I created this for version-3, as that's what I'm still using on my own projects.

@jhholm jhholm changed the title Temporary workaround for banner rework Workaround for MC791596 Sep 27, 2024
@juliemturner
Copy link
Collaborator

While we very much appreciate your effort, as indicated on the two issues that you reference in your PR we have decided as a team to no longer enhance/support the reverse engineered pages API that exists in V3 & V4 and have opened an issue to start implementing the new pages API in graph (#3137).

It looks like you have found a solution for your use case, and so I would suggest you build yourself a helper class to implement this work around. I'm going to close this PR per the comment above. I'm sorry I'm sure my response is disappointing, but the team feels this is the best direction for us to take given the complex nature of the pages api.

@heesungjang
Copy link

Hi @juliemturner ,

This issue is impacting one of our core product features, and I'm trying to resolve this from my end since this API is now deprecated, at least until the new API in Graph is available. Could I ask for your advice on possible approaches to create a wrapper class for the workaround? I can't think of a way to apply this workaround pull request other than forking the library (not even sure if that's possible).

I'm sure your days have been very busy, but when you have time, it would be really helpful to get your thoughts on this.

Thank you.

@jhholm
Copy link
Author

jhholm commented Oct 18, 2024

Please note that my proposed change is not a proper fix and there are use cases where it doesn't work. You are more than welcome to test and use my fork if you want, knowing that it might not work for you.

I would recommend looking into patch-package or if you're using pnpm pnpm patch .

The workflow is about the following:

  1. Preferably create your own fork of pnpjs, and apply the changes you need
  2. npm run build (for your pnpjs fork)
  3. npm run package (for your pnpjs fork)
  4. Install the same exact version of pnpjs to your project that you are using
  5. Copy the changes from your fork of pnpjs /dist to your node_modules @pnp/sp folder (in the project that would need this fix)
  6. npx patch-package @pnp/sp (in the project that would need this fix)
  7. Add the postinstall script "patch-package" (in the project that would need this fix)

Note that there is only one file changed in this pull request. Also note that the file is different between versions 3.x and 4.x. Should be trivial to port to 4.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants