percy / percy-storybook

Percy's Storybook SDK.
https://docs.percy.io/docs/storybook
MIT License
149 stars 45 forks source link

Duplicate snapshots with 3.3.0 (due to Emoji's) #201

Closed joscha closed 4 years ago

joscha commented 4 years ago

After upgrading from 3.2.0 to 3.3.0, I get:

Error:  StatusCodeError: 400 - {"errors":[{"status":"bad_request"},{"source":{"pointer":"/data/attributes/base"},"detail":"The name of each snapshot must be unique, and this name already exists in the build: 'pages.admin_cms.widgets.arities.map: string + string ' -- You can fix this by passing a 'name' param when creating the snapshot. See the docs for more info on identifying snapshots for your specific client: https://percy.io/docs"}]}
--
  | at new StatusCodeError (/workdir/web/node_modules/request-promise/node_modules/request-promise-core/lib/errors.js:32:15)
  | at Request.plumbing.callback (/workdir/web/node_modules/request-promise/node_modules/request-promise-core/lib/plumbing.js:104:33)
  | at Request.RP$callback [as _callback] (/workdir/web/node_modules/request-promise/node_modules/request-promise-core/lib/plumbing.js:46:31)
  | at Request.self.callback (/workdir/web/node_modules/request/request.js:185:22)
  | at Request.emit (events.js:210:5)
  | at Request.<anonymous> (/workdir/web/node_modules/request/request.js:1161:10)
  | at Request.emit (events.js:210:5)
  | at IncomingMessage.<anonymous> (/workdir/web/node_modules/request/request.js:1083:12)
  | at Object.onceWrapper (events.js:299:28)
  | at IncomingMessage.emit (events.js:215:7)
  | at endReadableNT (_stream_readable.js:1184:12)
  | at processTicksAndRejections (internal/process/task_queues.js:80:21) {

The story has a unicode character at the end: 👀 - potentially that could be part of the issue? I don't understand why it doesn't appear on 3.2.0 though.

Robdel12 commented 4 years ago

Hey Joscha! This is caused by the 👀 emoji in the story title. That gets encoded into the resource URL and that table currently doesn't support multi-byte UTF-8. The API then returns a 500 error which gets retried, causing the duplicate snapshot error.

The engineering team has a ticket to handle this, but it doesn't appear this will be worked on for at least a month or so. I'll keep this issue updated as things update/change 👍

joscha commented 4 years ago

Hi @Robdel12 we've had emojis in story titles for the last three years - ever since we started using Percy. It's close to impossible for us to move away as it would mean that ~1000 stories would change their name and lose history. Can you strip the emoji or slugify it? Or whatever else you did in the past to make it work prior 3.3.0? I understand the best fix is to keep the story name as-is and a database update takes time but I think you should be able to find an intermediate solution to allow existing projects to continue working whilst this DB upgrade hasn't been done yet?

Robdel12 commented 4 years ago

That makes sense! This is related to the newer way Storybook is encoding their URLs. The old way using the selectedKind and selectedStory params didn't encode the emoji into the URL. Their new way does encode emojis into the URL, which is why it goes 💥 now.

We can explore stripping or encoding the URL, but I'm wary of doing that. We would no longer match what's in the built storybook output and rewriting URLs could cause issues.

The easiest thing would to be hang back on v3.2.0, 3.3.0 pretty much only contains that change to the URL.

timhaines commented 4 years ago

@Robdel12 couple of thoughts here:

  1. Lets log a bug with engineering to handle emoji in url and snapshot names. This is something we should handle. Acknowledge it's going to take some effort and time.
  2. Can we double-check that the issue is in url fields not accepting emoji, and that the recent change hasn't accidentally dropped stripping emoji in snapshot names.
  3. If we didn't accidentally drop stripping emoji, can we re-add support as an option for "legacy" urls that use use selectedKind and selectedStory again so Canva isn't locked back on an obsolete version of our SDK please.
Robdel12 commented 4 years ago

Totally will create a Jira for it 👍 (cc: @faun since I'm a little fuzzy on the exact details of the scope)

The issue is we were using a legacy method for getting the stories URL in the SDK (up to v3.2.0). With v3.3.0 we switched to the new way to get the stories URL (in this PR: https://github.com/percy/percy-storybook/pull/166). This new way includes unencoded emojis in the URL, which is causing the API to fail when creating the snapshot. I wouldn't call v3.2.0 obsolete, I think it's okay to stay back one minor until our API can support storing emojis. If there is a major bug fix or more features added, I would agree for sure.

I wouldn't want to fork the logic in the SDK to use a deprecated format until the API can be fixed. That version of the SDK would live longer than the APIs bug. We also have an issue where it's not trivial to hand back storybook snapshots for debugging to customers because the URLs don't actually match what's in the static output. I'd like to not go further down that path by altering the URL, if possible. I want to take a look at encoding the URL from the new params ID -- if it works, then we can ship that.

joscha commented 4 years ago

is there any news on this one, please?

declanvong commented 4 years ago

Bump -- any updates for this?

christianscott commented 4 years ago

Hey @Robdel12 & @timhaines 👋 unfortunately we're still seeing this error intermittently on @percy/storybook@3.3.1. I can see the emoji is being encoded now but it doesn't seem to have solved it.

Error:  StatusCodeError: 400 - {"errors":[{"status":"bad_request"},{"source":{"pointer":"/data/attributes/base"},"detail":"The name of each snapshot must be unique, and this name already exists in the build: 'ui.referring.rewards.referral_panel: Story overview ' -- You can fix this by passing a 'name' param when creating the snapshot. See the docs for more info on identifying snapshots for your specific client: https://percy.io/docs"}]}

Here's an example request body from our logs:

{
  "data": {
    "type": "snapshots",
    "attributes": {
      "name": "ui.referring.rewards.referral_panel: Story overview 👀",
      "enable-javascript": true,
      "widths": [1280],
      "minimum-height": 800
    },
    "relationships": {
      "resources": {
        "data": [
          {
            "type": "resources",
            "id": "REDACTED_SINCE_I_DONT_KNOW_IF_THIS_IS_SENSITIVE",
            "attributes": {
              "resource-url": "/ui.referring.rewards.referral_panel:-story-overview.html?id=ui-referring-rewards-referral-panel--story-overview-%F0%9F%91%80",
              "mimetype": "text/html",
              "is-root": true
            }
          }
        ]
      }
    }
  }
}

I can still see the name property is unencoded - could that be what's causing problems?

joscha commented 3 years ago

we can still see this issue - any hints are appreciated!