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

fix: Avoid polyfill requirements #492

Merged
merged 5 commits into from
Mar 5, 2020

Conversation

anicholls
Copy link
Contributor

@anicholls anicholls commented Mar 2, 2020

We were unnecessarily using Object.entries in the check for an empty context in useTheme. This function requires a polyfill in IE 11, so I've switched it to use Object.keys instead, since this is equivalent for checking whether an object is empty.

Continued:
After a bit more exploration, we were using several functions that required polyfills. This PR now adds a lint check to ensure we don't do this in the future.

@cypress
Copy link

cypress bot commented Mar 2, 2020



Test summary

155 0 0 0


Run details

Project canvas-kit
Status Passed
Commit 25de081 ℹ️
Started Mar 4, 2020 8:52 PM
Ended Mar 4, 2020 8:56 PM
Duration 03:44 💡
OS Linux Ubuntu Linux - 16.04
Browser Electron 78

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@lychyi lychyi left a comment

Choose a reason for hiding this comment

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

This isn't the only place in our codebase that does Object.entries though, should this be something we polyfill? Or should we change everywhere?

@NicholasBoll
Copy link
Member

Hmm. Maybe we should update our tsconfig.json file to force compat mode with IE11. Polyfills would be used in Storybook and usually come with type definitions that opt into features like Promises.

Or perhaps an ESLint rule for detecting compat features? https://www.npmjs.com/package/eslint-plugin-compat

@anicholls anicholls changed the title fix(labs): Avoid polyfill requirement in useTheme check fix: Avoid polyfill requirements Mar 3, 2020
@anicholls
Copy link
Contributor Author

@NicholasBoll @lychyi After adding the linting rule, I found two more issues:
image

I fixed both, so we should be good now!

@lychyi lychyi added this to In Progress in Current Sprint (7/20 - 8/9) via automation Mar 4, 2020
@anicholls anicholls merged commit 6b8f2bb into Workday:master Mar 5, 2020
Current Sprint (7/20 - 8/9) automation moved this from In Progress to Done Mar 5, 2020
@anicholls anicholls deleted the fix-use-theme-check branch March 5, 2020 00:21
@anicholls anicholls mentioned this pull request Mar 11, 2020
lychyi pushed a commit that referenced this pull request Apr 13, 2020
* refactor(icon): Update to remove Object.entries usage

* feat: Add linting for browser compatible functions

* fix: Fix two browser compatibility issues

* refactor(icon): Simplify color lookup

Co-Authored-By: Cody Towstik <ctowstik@gmail.com>

* fix(core): Use Object.keys instead of Object.entries

Co-authored-by: Cody Towstik <ctowstik@gmail.com>
lychyi pushed a commit to lychyi/canvas-kit that referenced this pull request Apr 13, 2020
* refactor(icon): Update to remove Object.entries usage

* feat: Add linting for browser compatible functions

* fix: Fix two browser compatibility issues

* refactor(icon): Simplify color lookup

Co-Authored-By: Cody Towstik <ctowstik@gmail.com>

* fix(core): Use Object.keys instead of Object.entries

Co-authored-by: Cody Towstik <ctowstik@gmail.com>
lychyi added a commit that referenced this pull request Apr 13, 2020
* refactor(icon): Update to remove Object.entries usage

* feat: Add linting for browser compatible functions

* fix: Fix two browser compatibility issues

* refactor(icon): Simplify color lookup

Co-Authored-By: Cody Towstik <ctowstik@gmail.com>

* fix(core): Use Object.keys instead of Object.entries

Co-authored-by: Cody Towstik <ctowstik@gmail.com>

Co-authored-by: Alex Nicholls <anicholls3@gmail.com>
Co-authored-by: Cody Towstik <ctowstik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants