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

Core: Add polyfill for fetch #7401

Merged
merged 1 commit into from Aug 7, 2019
Merged

Core: Add polyfill for fetch #7401

merged 1 commit into from Aug 7, 2019

Conversation

adamdoyle
Copy link
Contributor

Issue: #7263

What I did

  • Added @storybook/core dependency on fetch-wg polyfill for fetch
  • Imported polyfill in lib/core/src/server/common/polyfill.js

How to test

Reproduce issue:

  1. Open Storybook baseline in IE11 (without PR)
  2. Press [F12] to open 'Developer Tools'
  3. Switch to Javascript console
  4. Reload browser
  5. Observe that console has the following warning: Failed to fetch latest version from server: TypeError: Object expected

Test the fix:

  1. Open Storybook with PR in IE11
  2. Repeat steps 2 - 4 from above
  3. Observe that instead of the warning about a failed fetch, there's now a CORS notice: XMLHttpRequest for https://storybook.js.org/versions.json?current=5.2.0-alpha.42 required Cross Origin Resource Sharing (CORS).

Note that the version check stores a timestamp for the latest successful attempt in local storage and won't check again until 24 hours past that timestamp. You can work around that in IE11 by invoking localStorage.clear() on the Javascript console.

Also note that in the latest version there also exists issue #7388 that also prevents IE from loading. Until that issue is resolved, IE11 still won't completely load. The author of #7263 was using v4.1.0, which didn't yet have this additional IE issue.

@vercel
Copy link

vercel bot commented Jul 12, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-adamdoyle-7263-add-fetch-polyfill.storybook.now.sh

@adamdoyle adamdoyle changed the title Add polyfill for fetch Core: Add polyfill for fetch Jul 12, 2019
@shilman shilman added this to the 5.1.x milestone Jul 12, 2019
@shilman shilman added patch:yes Bugfix & documentation PR that need to be picked to main branch core labels Jul 12, 2019
@emilio-martinez
Copy link
Contributor

I saw this warning earlier this week as well!

If I may, I'm going to play devil's advocate for a second, if it's okay:

  • Storybook uses fetch pretty sparingly in browser runtime:
    • 1 time in Storybook standalone (fetchLatestVersion, which this issue refers to)
    • And a 2 more times in add-ons
  • The large majority of global browser (>93%) usage has fetch support.
  • Usage in fetchLatestVersion fails gracefully, i.e., Storybook continues to work as expected.

To that I'd pose the following alternatives:

  • Conditionally loading the polyfill
  • Loading a smaller polyfill, e.g., unfetch.
  • Both of the above

@ndelangen
Copy link
Member

yeah If we can make these polyfills optional that would be a HUGE plus.

@ndelangen
Copy link
Member

Do you think you could @adamdoyle

@adamdoyle
Copy link
Contributor Author

Both unfetch and whatwg-fetch already do something like this out of the box:

if (!self.fetch) {
   self.fetch = /* ... polyfill */;
}

So they're already "conditional" in that sense. Unless I'm mistaken, anything beyond that would require either (1) a compile-time flag to exclude it from the webpack output, or (2) a dynamic import.

I can switch it to unfetch, though, if that's what you guys want.

@emilio-martinez
Copy link
Contributor

Yep, dynamic import is where my mind was at, especially since Webpack has first class support for it.

@adamdoyle
Copy link
Contributor Author

I could throw together something like this (they're using parcel, but same idea).

I just want to make sure you (@emilio-martinez) and @ndelangen are on board with the general idea before I put together a PR.

@ndelangen
Copy link
Member

Yea the imperative imports import('./fetch-polyfill') should work just fine.

@ndelangen ndelangen modified the milestones: 5.1.x, 5.2.0 Jul 23, 2019
@bhargavshah
Copy link

Hello All,
Thank you for looking at this issue. If everything seems all right in this PR can we merge it please?
How long does it usually take release a bugfix after it's merged? 🤔

@shilman
Copy link
Member

shilman commented Aug 5, 2019

@bhargavshah I think we're waiting on a lighter polyfill, though tbh I'm fine with the current fix (cc @adamdoyle). As for releasing, we usually release it on next within 1-3 days of merge. Stable usually gets updated once every week or two.

@vercel vercel bot temporarily deployed to staging August 6, 2019 15:17 Inactive
- conditionalize fetch polyfill
- switch from whatwg-fetch to unfetch
@adamdoyle
Copy link
Contributor Author

@ndelangen @emilio-martinez updated to use dynamic import and switched to unfetch, as requested

should be ready for review

@chasemccoy
Copy link

It would be awesome to get this shipped! My team hasn't been able to test components in IE for a while 😭

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWESOME

@shilman shilman merged commit 61ff1a7 into storybookjs:next Aug 7, 2019
shilman added a commit that referenced this pull request Aug 13, 2019
Core: Add polyfill for fetch
@shilman shilman removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants