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 whitespace creation #74

Merged
merged 9 commits into from
May 19, 2017
Merged

fix whitespace creation #74

merged 9 commits into from
May 19, 2017

Conversation

yoshuawuyts
Copy link
Member

@yoshuawuyts yoshuawuyts commented May 17, 2017

This should fix #72

patch notes

  • now creates strings on the server, leading to a 10x speed improvement for rendering
  • detects if in Electron, Node, or browser and runs the right version
  • fixes random text node creation in lists

@yoshuawuyts
Copy link
Member Author

Ok, so right now things are not solved. The current state is:

  • browser tests pass, behavior is patched
  • server tests fail, we now rely on .lastChild in the critical path and min-document does not recognize that
  • this means that running code on the server could already fail, but it wasn't caught by the tests

What I'm thinking we should do:

We should get a 10x server render improvement in the process, but also do a semver major release. With that happening we could get cheeky and also add #69.

I have a feeling this is a fair amount of changes, so input would be greatly appreciated! Thanks heaps!


IRC dump

02:55 <yoshuawuyts> URGH
02:55 <yoshuawuyts> ok, so I got this whole mad thing working in the browser - fixed the regression
02:55 <yoshuawuyts> (talking about `bel`)
02:55 <yoshuawuyts> but now the server is failing
02:56 <yoshuawuyts> and worse: it's failing because of a pre-existing bug in `bel` - e.g. min-document is an incomplete DOM implementation, and stuff like .nodeValue and .lastChild does not exist
02:57 <yoshuawuyts> so either: 1) we do a revert and pretend nothing changed. This will disallow child-node reordering tho. Or 2) we merge and break server rendering
02:57 <yoshuawuyts> I'm not thrilled about either option
02:59 <yoshuawuyts> well, technically we're not breaking server rendering - it's already broken - it just makes it, err, well more apparent
03:00 <yoshuawuyts> could be a cool opportunity to complete move to https://github.com/shuhei/pelo btw
03:00 <yoshuawuyts> option 3?
03:00 <yoshuawuyts> (anyone got thoughts on this?)
03:05 <sethvincent> would it be worth working on min-document or an alternative?
03:07 <yoshuawuyts> sethvincent: pelo uses string concatenation which provides a 20x speed improvement over object based approaches. I think if we're going to change anything specifically for server stuff that'd be the clearest path
03:07 <sethvincent> hmm, that makes sense

@yoshuawuyts
Copy link
Member Author

woot, it works!

@yoshuawuyts
Copy link
Member Author

Ok, so we're now running pelo in node / electron main process, and regular ol' bel code in electron renderer / browser. This means we got a 10x or so perf improvement for rendering in Node. Yay!

Semver major because the intermediate representation on the server also changed. Thanks!

@yoshuawuyts yoshuawuyts merged commit 672ebab into master May 19, 2017
@yoshuawuyts yoshuawuyts deleted the fix-whitespace branch May 19, 2017 14:39
@mantoni
Copy link
Contributor

mantoni commented May 19, 2017

That little whitspace fix got out of hand, right? 😆

@yoshuawuyts yoshuawuyts mentioned this pull request May 19, 2017
@yoshuawuyts
Copy link
Member Author

v5.0.0

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.

Spaces are incorrectly stripped
2 participants