Skip to content

feat(client): introduce core-js - #1950

Closed
hiroppy wants to merge 13 commits into
nextfrom
feature/add-core-js
Closed

feat(client): introduce core-js#1950
hiroppy wants to merge 13 commits into
nextfrom
feature/add-core-js

Conversation

@hiroppy

@hiroppyhiroppy commented Jun 1, 2019

Copy link
Copy Markdown
Contributor
  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

modify only

Motivation / Use-Case

#1948 (comment)

Breaking Changes

no

Additional Info

Comment threadclient-src/default/polyfill.js Outdated
'use strict';

require('core-js/features/string/includes');
require('core-js/features/array/includes');

@alexander-akaitalexander-akaitJun 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We pollute global namespace 😞 We should use named import like in docs https://github.com/zloirock/core-js

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

sorry, why?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we need setup support import/export in our babel config for client too

@alexander-akaitalexander-akaitJun 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using require('core-js/features/array/includes'); modify Array.prototype, so if developer already have modified Array.prototype we break can break their app. Using import arrayIncludes from 'core-js/features/array/includes' doesn't modify Array.prototype so it is safe to use

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense. ok, I'll change

@hiroppy
hiroppy changed the base branch from master to nextJune 10, 2019 18:04
@hiroppy
hiroppyforce-pushed the feature/add-core-js branch from ddfff11 to 3ea39adCompareJune 10, 2019 23:30
@hiroppy

hiroppy commented Jun 10, 2019

Copy link
Copy Markdown
ContributorAuthor

@evilebottnawi updated, PTAL

@codecov

codecovBot commented Jun 10, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1950 into next will increase coverage by 0.01%.
The diff coverage is 75%.

Impacted file tree graph

@@ Coverage Diff @@## next #1950 +/- ##
==========================================
+ Coverage 92.61% 92.62% +0.01% 
==========================================
Files 29 30 +1 Lines 1124 1126 +2 Branches 325 325 ==========================================
+ Hits 1041 1043 +2 
Misses 79 79 Partials 4 4
Impacted FilesCoverage Δ
client-src/default/polyfill.js100% <100%> (ø)
client-src/default/index.js91.56% <50%> (ø)⬆️

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 9c5f6bb...3ea39ad. Read the comment docs.

@alexander-akaitalexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Before merge please manually check what we don't modify global built-in (just looks in client and this file should not have any String.prototype or Array.prototype)

@hiroppy
hiroppyforce-pushed the next branch 4 times, most recently from 589bbdf to eeb3fd2CompareJune 15, 2019 15:56
@hiroppy
hiroppyforce-pushed the next branch 3 times, most recently from f469dc3 to 534dedeCompareJune 24, 2019 15:29
@hiroppy

Copy link
Copy Markdown
ContributorAuthor

I'll create other new pr.

@hiroppyhiroppy closed this Dec 5, 2019
@hiroppy
hiroppy deleted the feature/add-core-js branch December 5, 2019 06:12
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@hiroppy@alexander-akait