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

Small optimization in render hyperscript.js #2064

Merged
merged 5 commits into from May 18, 2018

Conversation

Projects
None yet
5 participants
@magikstm
Copy link
Contributor

magikstm commented Dec 30, 2017

Description

No functional change. I read code and noticed it.

Motivation and Context

Minor. Feel free to pull or not.

How Has This Been Tested?

Locally.

Types of changes

Optimization.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@magikstm magikstm requested review from isiahmeadows and pygy as code owners Dec 30, 2017

@Gandalf-the-Bot

This comment has been minimized.

Copy link
Contributor

Gandalf-the-Bot commented Dec 30, 2017

Warnings
⚠️

Please add an entry to docs/change-log.md.

Generated by 🚫 dangerJS

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

LGTM mod a small nit

@@ -108,6 +104,7 @@ function hyperscript(selector) {
var normalized = Vnode.normalizeChildren(children)

if (typeof selector === "string") {
var cached = selectorCache[selector] || compileSelector(selector)

This comment has been minimized.

@isiahmeadows

isiahmeadows Dec 30, 2017

Collaborator

Mind combining this with the next line, to save a temporary variable? (Your patch is more so optimizing space than speed, BTW.)

magikstm added some commits Dec 30, 2017

Small optimization in render hyperscript.js
Squashed commits:

[5b10329] Small optimization in render hyperscript.js
@magikstm

This comment has been minimized.

Copy link
Contributor

magikstm commented Dec 30, 2017

Thanks for the review. I adjusted it.

I'm having trouble to squash commits in github with the command-line tools or SourceTree. :(

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Dec 30, 2017

@magikstm The "Sloppy mode" comment was paired with the line you moved. I don't even remember why it's there (I used to, I thought I had made it more meaningful but that PR was probably not merged).

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Dec 30, 2017

@magikstm

This comment has been minimized.

Copy link
Contributor

magikstm commented Dec 30, 2017

Thanks. I just removed the comment.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Dec 30, 2017

@magikstm If you're okay working from the CLI, there's git log to display info about commits, and git rebase ${ref} -i/git rebase ${ref} --interactive to interactively modify the history. I do that quite frequently when working with patch sets to trim the commits down to something a little more logical and workable.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Dec 31, 2017

@isiahmeadows I'm not against dropping IE 9 (and 10) support, given how marginal it has become, but I didn't know this was planned.

OTOH given the onpopstate bugs in IE 11 (IIRC) I'm not sure it buys us much.

@magikstm

This comment has been minimized.

Copy link
Contributor

magikstm commented Dec 31, 2017

@isiahmeadows I'm unfortunately a bit limited with the Git CLI. I use it a bit, but usually use SourceTree. I'm not that used to squashing commits. It's a feature of Git I have to learn more.

Could you do it before a merge or after it?

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Dec 31, 2017

@barneycarroll
Copy link
Member

barneycarroll left a comment

This is much nicer IMO 👍

@pygy pygy merged commit 6097cfb into MithrilJS:next May 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 18, 2018

Here we go!

Thanks @magikstm

@pygy pygy referenced this pull request May 18, 2018

Merged

Do not normalise component children on ingestion #2155

4 of 10 tasks complete

barneycarroll added a commit that referenced this pull request May 29, 2018

barneycarroll added a commit that referenced this pull request May 29, 2018

Do not normalise component children on ingestion (#2155)
* Do not normalise component children on ingestion

* Don't normalise vnode children

* Component hyperscript tests: children aren't normalised

* test, not text

* Update change log: #2155 & #2064

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Oct 12, 2018

Small optimization in render hyperscript.js (MithrilJS#2064)
* Small optimization in render hyperscript.js

* Remove temporary variable

* Small optimization in render hyperscript.js

Squashed commits:

[5b10329] Small optimization in render hyperscript.js

* Remove sloppy mode comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment