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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

tweak defining proto #95

Closed
wants to merge 2 commits into from
Closed

tweak defining proto #95

wants to merge 2 commits into from

Conversation

stevemao
Copy link
Contributor

@stevemao stevemao commented Jan 5, 2016

No description provided.

"resolve-from": "^1.0.0",
"semver": "^4.3.3",
"resolve-from": "^2.0.0",
"semver": "^5.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Don't do unrelated changes. Bumping deps should be a separate PR.

@sindresorhus sindresorhus changed the title tweak defining proto and bump deps tweak defining proto Jan 5, 2016
@sindresorhus
Copy link
Member

The description field is there for a reason ;) Could you describe what you've changed and why?

@Qix-
Copy link
Member

Qix- commented Jan 5, 2016

All this PR seems to do is to remove the function that generates the Chalk prototype in lieu of a simple object. There are reasons why we do this. Could you explain the decision behind changing this?

@jbnicolai
Copy link
Contributor

The proposed implementation is simpler, and has less code.. so I'm all for 馃槈

@Qix- what's the reason? I suspect it's an artifact of refactor-fu back when trying to make the whole chained-call implementation performant, but perhaps I'm unaware of the slight difference between the current implementation's behaviour and that of the PR.

All tests seem to pass though, and running the benchmark 10 times on both reveals no difference in performance.

for i in {0..10}; do npm run bench; done | grep Elapsed

on master:

  Elapsed: 5,773.57 ms
  Elapsed: 5,819.39 ms
  Elapsed: 5,974.76 ms
  Elapsed: 5,678.02 ms
  Elapsed: 5,809.12 ms
  Elapsed: 5,790.01 ms
  Elapsed: 5,809.15 ms
  Elapsed: 5,724.53 ms
  Elapsed: 5,870.90 ms
  Elapsed: 5,789.78 ms
  Elapsed: 5,970.74 ms

on stevemao/cleanup:

  Elapsed: 5,770.72 ms
  Elapsed: 5,770.93 ms
  Elapsed: 5,917.21 ms
  Elapsed: 5,872.29 ms
  Elapsed: 5,757.63 ms
  Elapsed: 5,804.15 ms
  Elapsed: 5,733.41 ms
  Elapsed: 5,950.70 ms
  Elapsed: 5,967.84 ms
  Elapsed: 5,961.10 ms
  Elapsed: 5,798.23 ms

return ret;
}

defineProps(Chalk.prototype, init());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to just make the code simpler. and the logic is pretty much the same as generating the styles above so I'm reusing it.

@stevemao
Copy link
Contributor Author

stevemao commented Jan 5, 2016

I'll make the changes tonight :)

@stevemao
Copy link
Contributor Author

stevemao commented Jan 6, 2016

I broke this into three PRs (#96, #97)

@stevemao
Copy link
Contributor Author

stevemao commented Jan 6, 2016

Any questions/problems with this PR?

@sindresorhus
Copy link
Member

I'm busy at the moment, will try to review in a few days.

@jbnicolai @Qix- Looks good to you?

@Qix-
Copy link
Member

Qix- commented Jan 7, 2016

Yeah this looks fine. I misread what happened with the removal of the function.

@sindresorhus
Copy link
Member

@stevemao Awesome! Thank you. I dig PRs that simplify the 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.

None yet

4 participants