newrelic / docs-website

Source code for @newrelic docs. We welcome pull requests and questions on our docs!
https://docs.newrelic.com
Other
173 stars 1.25k forks source link

Disabling all 🍪 cookies breaks site components #2293

Closed jpvajda closed 3 years ago

jpvajda commented 3 years ago

Relates to:

2287

Disabling all cookies effects components in odd ways, like collapsers & landing page tiles & navigation, & the data dictionary page.

note given this was found with #2287 it may be related to the free account button not rendering when cookies are disabled.

Steps to repro:

Screen Shot 2021-05-13 at 3 29 33 PM

Acceptance Criteria

jpvajda commented 3 years ago

We discussed timeboxing this to see if there is a quick resolution but browsers do make it clear that by disabling all cookies you may break website functionality.

LizBaker commented 3 years ago

I played around with this a bit in the theme and it seems react-spring (our animation library) doesn't function when cookies are disabled, so any component with any kind of animation will break

jpvajda commented 3 years ago

@LizBaker interesting finding.. I wonder if there is anything we can report to React-Spring? perhaps file an issue? https://github.com/pmndrs/react-spring/issues?q=is%3Aissue+is%3Aopen+

aswanson-nr commented 3 years ago

This appears to be caused by interacting with the localStorage API. When all cookies are disabled, that API throws an error when we try to set a new item and appears to completely throw us out of the JS execution in the browser. I'm further convinced that this is the root cause because a lack of JS would make the site operate in the way we are seeing. If no JS is running, then the collapsers wouldn't open, the nav wouldn't work properly, the search wouldn't work, etc.

Using Safari with all cookies blocked I ran into this error, along with a few other similar ones.

Screen Shot 2021-05-19 at 4 03 19 PM

After digging deeper into this error, I noticed that it appears to be coming from some code around setting the light/dark theme.

Screen Shot 2021-05-19 at 4 04 10 PM

This code appears to be coming from the use-dark-mode npm package. It has an open issue for this problem https://github.com/donavon/use-dark-mode/issues/53

I didn't see a quick fix, so I didn't dig any deeper besides checking to see if the package had anything about this use case in its documentation (it doesn't). If this is the root cause of this issue, then we will need to file an issue with the use-dark-mode package or move to handling the theme mode internally.

jpvajda commented 3 years ago

@aswanson-nr that's a good assessment! Maybe we discuss in Standup parking lot?

aswanson-nr commented 3 years ago

@LizBaker and @jpvajda, I went through the theme and disabled all uses of the use-persisted-state library and I was able to get the demo site running! The library does not seem to be able to handle the case where users have disabled all cookies. It should be possible for us to keep the majority of the site operating normally by replacing the use-persisted-state library with another one that will fallback to an in-memory store if localstorage isn't available. This is also something we could consider implementing ourselves, since the API is very simple.

aswanson-nr commented 3 years ago

This library, use-local-storage-state, looks like a great candidate to replace use-persisted-state. There's even a section on handling edge cases when localstorage isn't available, https://www.npmjs.com/package/use-local-storage-state#is-persistent-example.

jpvajda commented 3 years ago

Nice!

aswanson-nr commented 3 years ago

Proposed AC:

EDIT: This is not quite as simple as I initially thought. The use-dark-mode library, which we use to control the light/dark mode of the theme, relies on the use-persisted-state library. To get around this, we would need implement our own or find a new library.

jpvajda commented 3 years ago

@aswanson-nr curious why this is blocked? Did we decide to just punt on this for now, and focus on getting the free account to render without cookies being enabled?

aswanson-nr commented 3 years ago

@jpvajda Exactly that! I wanted to talk through that decision in standup

aswanson-nr commented 3 years ago

Deep dive: Local storage is used 4 places in the theme

  1. user id storage
    • Currently uses a custom implementation with local storage
  2. the use-dark-mode library
    • uses the use-persisted-state library under the hood, this library does not work well when cookies are disabled.
    • the default storage provider can be overridden to use an in-memory store.
  3. on-client-entry to check if dark mode is used (seems to just exist for reporting)
    • adding a try/catch block around the local storage calls allows the client to continue working fine when cookies are disabled.
  4. the announcement banner
    • currently uses use-persisted-state

Proposed changes:

  1. Update this to use the use-local-storage-state.
  2. Check to see if local storage is available, then pass in an in-memory store if it isn't.
  3. Add the try catch around access local storage.
    • What should the NR custom attribute default to?
  4. Update this to use the use-local-storage-state.

I'm going to submit a PR for these changes, since I basically made all these changes during testing.