pnp / sp-formatter

SharePoint formatter Chromium Edge and Google Chrome browser extension
MIT License
35 stars 6 forks source link

Fixes #3

Closed thechriskent closed 3 years ago

thechriskent commented 3 years ago

I noticed SP Formatter was no longer working for me starting earlier this week. I've been meaning to dive further into your awesome code for a while so this was a nice excuse. I found a few issues that I corrected (details below), but the biggest issue was ultimately a relatively simple fix.

Page Context Data Clone Error

image

The onGetSPPageContextInfo event was sending the entire _spPageContextInfo. With some new additions to the context, the emit function was failing because of how postMessage implements it's data clone. This error was killing the whole extension. To correct, I adjusted the response data in the SPPageContextProvider to only return the isSPO value from the context instead of the entire context. This will mean additional context info may need to be explicitly mapped but should prevent unexpected failures as Microsoft makes changes to the _spPageContextInfo.

I did NOT touch the legacyContext.

Other Changes

These other changes are not critical to SP Formatter's functioning. So it's cool if you don't feel these are right for the extension and just want to implement the above fix and reject the rest.

Updated to @FluentUI/react 8 and react/react-dom 17

React 16 is affected by the SharedArrayBuffer changes and it is causing issues (warnings for now but potential failures soon) in the extension. This issue (within the Scheduler dependency in React) was corrected in 17.0.2.

image

I went ahead and moved the extension to react/react-dom to 17.0.2 and removed office-ui-fabric-react in favor of the new @fluentui/react 8.12.0.

I have run into zero issues with this change, but this is likely the most controversial item in this pull request so feel free to reject.

useDarkMode initial setting error

When extension settings are first returned, if there isn't yet settings stored the settings returned are null. This is fine for the enhanceFormatterEnabled setting as the default value for this is handled. Unfortunately, the useDarkMode setting is referenced before being set and this was throwing an error and making it impossible to toggle the setting. I added defaultSettings and when no settings are retrieved from storage these are used and the defaults saved.

Added more detail to the Web Extension README

Added additional notes and links for getting the development environment setup based on my experience.

Minor Junk

I added some more logging in the PromiseTimeout to help troubleshoot the issues with the page context. In doing so I found that the message used in the getSpPageContextInfo timeout initiated by the ContentManager was using the wrong label.

s-KaiNet commented 3 years ago

Hi Chris, thank you for your great input here!

A few users reported that the extension doesn't work for them, however, it works for me on two different tenants with First Release settings, thus it was quite difficult to isolate the problem. Now it looks like they have the same problem as you - page context info object serialization issue.

I will take a closer look at the PR tomorrow.

s-KaiNet commented 3 years ago

I manually picked almost all of your commits except Fluent UI and React updates. I have a feeling that it might cause issues for SP 2019, thus it needs to be tested separately. Also, since SharePoint is still on Fabric 7, maybe it makes sense to hold on for a while with these updates. Everything else was merged (except color settings, perhaps it's better to have such settings as user config and not project). Actually, SP 2019 introduced quite a lot of problems, because SPO moves forward and SP 2019 was pushed to a legacy folder (that's the reason why isSpo was added). In the future, it will be even more different. Sometimes I even think about releasing a separate extension for SP 2019 specifically. It will make SPO extension more lightweight.

Thank you again Chris, I can imagine it was not a simple journey to dive into the extension with all the complicated messaging between popup scripts, background scripts, content scripts and finally page scripts :). But this is the way how chrome enables the communication between different extension objects. The communication part is difficult in sp-formatter and can be improved, however, I haven't invented how :)

Later today I will push packages to Chrome and Edge. Usually, it takes from few hours up to one day for them to approve a newer version. Hopefully these fixes unblock some customers who experience problems.

thechriskent commented 3 years ago

Sounds great! Honestly, the React/FluentUI updates should have been put in a separate PR anyway. I went down a rabbit hole thinking the SharedArrayBuffer was the issue, but I should have backtracked once it became obvious that wasn't the problem this time.

Totally forgot I changed the dumb color settings. Yeah, those are just for me 🤦‍♂️.

Thank you for taking the time to go through and clean things up for me! I love this extension and I use it all the time! The VSCode integration is genius and makes processing List Formatting PRs so much easier!