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

Refactor project #844

Merged
merged 2 commits into from
Feb 7, 2018
Merged

Refactor project #844

merged 2 commits into from
Feb 7, 2018

Conversation

gregberge
Copy link
Collaborator

@gregberge gregberge commented Feb 6, 2018

  • Refactor all project
  • Remove Lerna
  • Simplify code

@codecov-io
Copy link

codecov-io commented Feb 6, 2018

Codecov Report

Merging #844 into next will decrease coverage by 0.09%.
The diff coverage is 37.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             next     #844     +/-   ##
=========================================
- Coverage   86.21%   86.12%   -0.1%     
=========================================
  Files          25       30      +5     
  Lines         573      598     +25     
  Branches      141      139      -2     
=========================================
+ Hits          494      515     +21     
- Misses         59       67      +8     
+ Partials       20       16      -4
Impacted Files Coverage Δ
src/internal/reactUtils.js 84.61% <ø> (ø)
src/internal/stack/hydrateLegacyStack.js 0% <ø> (ø)
src/hot.dev.js 91.42% <ø> (ø)
src/proxy/constants.js 100% <ø> (ø)
src/reconciler/proxyAdapter.js 94.44% <ø> (ø)
src/internal/stack/hydrateFiberStack.js 100% <ø> (ø)
src/reactHotLoader.js 100% <ø> (ø)
src/AppContainer.prod.js 100% <ø> (ø)
src/global/modules.js 100% <ø> (ø)
test/proxy/helper.js 100% <ø> (ø)
... and 39 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 bf519d4...15200cc. Read the comment docs.

@gregberge gregberge force-pushed the wip-refactoring branch 2 times, most recently from 4513144 to 06acf6b Compare February 6, 2018 10:15
"devDependencies": {
"babel-cli": "^6.7.5",
"babel-core": "^6.7.6",
"babel-eslint": "^8.2.1",
"babel-jest": "^22.1.0",
"babel-plugin-dynamic-import-node": "^1.2.0",
"babel-plugin-external-helpers": "^6.22.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've removed this plugin from rollup config, but not from the package json.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why?

Copy link
Collaborator

@theKashey theKashey Feb 6, 2018

Choose a reason for hiding this comment

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

It has a side effect on the final bundle, causing this bug - storybookjs/storybook#1320

So, I've just removed it, and double check that bundle is ok-ish. Lets dont introduce some external stuff, the final customer should also inject in his code.

@@ -99,7 +99,7 @@ describe('hot (dev)', () => {
}, 1)
})

it('should trigger error in unmount in opened state', () => {
it.only('should trigger error in unmount in opened state', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

My bad.

@@ -148,21 +148,21 @@ function inject(target, currentGeneration, injectedMembers) {
key,
`(function REACT_HOT_LOADER_SANDBOX () {
var _this = this; // common babel transpile
var _this2 = this; // common babel transpile
var _this2 = this; // common babel transpile
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is ☠️, I know that _this3 exists.
In ideal we should or inject regenerate in a constructor, to make these variables visible, or detect the error on regenerate, and ask to disable arrow function transpiling.

- Refactor all project
- Remove Lerna
- Simplify code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants