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

[addon-actions] Check result of getPropertyDescriptor for IE11 #2428

Merged
merged 4 commits into from
Dec 5, 2017

Conversation

jgoz
Copy link
Contributor

@jgoz jgoz commented Dec 4, 2017

Issue: When using action-addons in IE11, it would throw "Unable to get property 'configurable' of undefined or null reference" because apparently IE returns undefined from Object.getPropertyDescriptor when called on a function.

What I did

Check result of Object.getPropertyDescriptor before checking .configurable.

How to test

Load storybook that uses actions in IE11 and verify that the iframe loads.

Is this testable with jest or storyshots? No, this is IE-specific behaviour.

Does this need a new example in the kitchen sink apps? No

Does this need an update to the documentation? No

@codecov
Copy link

codecov bot commented Dec 4, 2017

Codecov Report

Merging #2428 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2428      +/-   ##
==========================================
+ Coverage   16.64%   16.65%   +0.01%     
==========================================
  Files         302      302              
  Lines        7769     7770       +1     
  Branches      786      776      -10     
==========================================
+ Hits         1293     1294       +1     
- Misses       5821     5879      +58     
+ Partials      655      597      -58
Impacted Files Coverage Δ
addons/actions/src/preview.js 55.55% <100%> (+2.61%) ⬆️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/vue/src/server/babel_config.js 0% <0%> (-77.42%) ⬇️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/api/index.js 0% <0%> (ø) ⬆️
...ddons/actions/src/components/ActionLogger/index.js 5.26% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/search_box.js 23.52% <0%> (ø) ⬆️
lib/components/src/navigation/menu_link.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Elements.js 0% <0%> (ø) ⬆️
...mments/src/manager/components/CommentItem/index.js 0% <0%> (ø) ⬆️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1db950d...1813b89. Read the comment docs.

// This condition is true in modern browsers that implement Function#name properly
const canConfigureName = Object.getOwnPropertyDescriptor(handler, 'name').configurable;
const canConfigureName = nameDescriptor && nameDescriptor.configurable;
Copy link
Member

@Hypnosphi Hypnosphi Dec 4, 2017

Choose a reason for hiding this comment

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

We can also try !nameDescriptor || nameDescriptor.configurable. As far as I remember, IE11 had no problems with overriding function name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I tested this suggestion in IE11 and it worked. Pushed a new commit, please have a look.

@Hypnosphi Hypnosphi merged commit 267b90a into storybookjs:master Dec 5, 2017
@jgoz jgoz deleted the fix-actions-undefined-descriptor branch December 5, 2017 16:59
BernieSumption added a commit to newsuk/times-components that referenced this pull request Dec 19, 2017
3.2.18 includes a fix for a bug that broke storybook in IE11:
storybookjs/storybook#2428
craigbilner pushed a commit to newsuk/times-components that referenced this pull request Dec 20, 2017
3.2.18 includes a fix for a bug that broke storybook in IE11:
storybookjs/storybook#2428
@ncomont ncomont mentioned this pull request Dec 21, 2017
4 tasks
BernieSumption added a commit to newsuk/times-components that referenced this pull request Dec 22, 2017
3.2.18 includes a fix for a bug that broke storybook in IE11:
storybookjs/storybook#2428
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.

2 participants