Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

chore(package): add polyfills to correctly work in IE11 #868

Merged
merged 2 commits into from
Feb 11, 2019

Conversation

layershifter
Copy link
Member

Fixes #769.

@codecov
Copy link

codecov bot commented Feb 8, 2019

Codecov Report

Merging #868 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #868   +/-   ##
=======================================
  Coverage   92.99%   92.99%           
=======================================
  Files          21       21           
  Lines         728      728           
  Branches       69       73    +4     
=======================================
  Hits          677      677           
  Misses         51       51

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 bf5ed14...e41a883. Read the comment docs.

@@ -9,6 +9,7 @@ export const babelConfig = {
['transform-typescript', { isTSX: true }],
'transform-classes',
],
presets: ['es2015'],
Copy link
Member Author

Choose a reason for hiding this comment

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

Need this to run examples in IE11

@@ -26,6 +26,7 @@
</script>
</head>
<body>
<script src="https://cdn.jsdelivr.net/npm/@babel/polyfill@7/dist/polyfill.min.js"></script>
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -145,7 +145,7 @@
"react-hot-loader": "^4.1.3",
"react-router": "^4.1.2",
"react-router-dom": "^4.1.2",
"react-source-render": "^2.0.0-beta.4",
"react-source-render": "2.0.0-beta.6",
Copy link
Member Author

Choose a reason for hiding this comment

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

IE11 doesn't support the object destruction, see changes in fixtures: layershifter/react-source-render@264e05a

@@ -581,7 +581,7 @@ class Dropdown extends AutoControlledComponent<Extendable<DropdownProps>, Dropdo
statusDiv.setAttribute('role', 'status')
statusDiv.setAttribute('aria-live', 'polite')
statusDiv.setAttribute('aria-relevant', 'additions text')
Object.assign(statusDiv.style, screenReaderContainerStyles)
_.assign(statusDiv.style, screenReaderContainerStyles)
Copy link
Member Author

Choose a reason for hiding this comment

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

https://lodash.com/docs/4.17.11#assign

Note: This method mutates object and is loosely based on Object.assign.

Copy link
Contributor

Choose a reason for hiding this comment

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

would we need to ensure that Object.assign won't be used anywhere in the codebase? I would rather expect that we will introduce polyfill that will ensure that even if Object.assign is used somewhere in the codebase, it will be handled correctly. Currently we are just introducing implicit rule of using _.assign vs Object.assign that will just serve as short-term patch.

I do understand that we may introduce lint rules, but for this particular case it seems that much easier solution will be just to introduce a polyfill - this will ensure that all the previous code would work without any additional changes necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

I support too adding polyfill, it will be better for the developers. And yep, we can add additional polyfills if there are other constraints in the future.

@layershifter layershifter added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. and removed 🚧 WIP labels Feb 11, 2019
Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

We should enforce IE11 compatibility by lint rules

@layershifter layershifter changed the title fix(lib): use _.assign instead of Object.assign to work in IE11 chore(package): add polyfills to correctly work in IE11 Feb 11, 2019
@layershifter layershifter merged commit 25c90cf into master Feb 11, 2019
@layershifter layershifter deleted the fix/work-in-ie11 branch February 11, 2019 14:12
miroslavstastny added a commit that referenced this pull request Feb 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants