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

9.0.0 #52

Merged
merged 25 commits into from
Apr 11, 2018
Merged

9.0.0 #52

merged 25 commits into from
Apr 11, 2018

Conversation

thysultan
Copy link
Member

This adds the new context api, improves some implementation details and makes changes that would make it easier to implement #51.

The current commit includes changes that make Error Boundaries more inline with #50

There are still some tests that fail/skipped, namely, the Factory test, but that is intentional because i'm still figuring out if any other config hooks need to change.

@coveralls
Copy link

coveralls commented Mar 4, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 4dc8af7 on 9.0.0 into dd9fd99 on master.

toString: {value: toString},
toStream: {value: toStream}
})
Element.prototype.toJSON = toJSON

Choose a reason for hiding this comment

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

note: now enumerable: true

@@ -204,10 +182,10 @@ function getDOMType (element, xmlns) {
case 'math':
return 'http://www.w3.org/1998/Math/MathML'
case 'foreignObject':
return ''
return
Copy link

@Zolmeister Zolmeister Mar 5, 2018

Choose a reason for hiding this comment

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

prefer null if result value is to be used

@thysultan
Copy link
Member Author

thysultan commented Mar 5, 2018

The latest commit is a POC related to this RFC: reactjs/rfcs#29

i.e

async function * TestFunc() {
	while (true) {
		yield new Promise((resolve) => {
			setTimeout(() => {
				resolve(h('li', this.state.index))
			}, 500)
		})
	}
}

@@ -41,62 +41,50 @@ function childrenArray (children) {
* @param {function} callback
* @param {*} thisArg
*/
function childrenEach (children, callback, thisArg) {
function eachChildren (children, callback, thisArg) {
if (children != null)

Choose a reason for hiding this comment

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

should this be an assert?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the question?

Choose a reason for hiding this comment

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

As in, this function should never be called with children = null, and if so, it implies an error somewhere else in the code.
i.e. throw if children != List()

Copy link
Member Author

@thysultan thysultan Mar 5, 2018

Choose a reason for hiding this comment

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

This is for the following.

dio.Children.forEach(this.props.children)

Since this.props.children is an opaque data-structure it could be an element, undefined or an array, this deals with the undefined/null case like the way React deals with it.

@@ -216,7 +226,7 @@ function createElement (type, config) {
if (isArray(config))
break
case Object:
if (typeof config.then === 'function')
if (thenable(config))

Choose a reason for hiding this comment

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

rename to isThenable

* @param {(object|string)} exception
*/
function printErrorException (exception) {
try { console.error(exception.toString()) } catch (e) {} finally { return exception }

Choose a reason for hiding this comment

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

catch is optional with finally

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that was the case as well but found out that these two produce different results.

try { throw '' } finally {}

throws.

try { throw '' } catch (e) {} finally {}

shallows the error

Choose a reason for hiding this comment

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

I see, otherwise (if the error happened during toString) it would re-throw

Copy link
Member Author

Choose a reason for hiding this comment

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

or console/console.error might not exist/might have been subbed to one that throws etc.

Choose a reason for hiding this comment

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

There is optional catch syntax proposal in stage 3, if you are interested in setting it up in your Babel config:

try { throw '' } catch {} finally {}

invokeErrorBoundary(host, err, 'on'+type+':'+getDisplayName(callback.handleEvent || callback), SharedErrorPassive)
if (value && owner[SymbolComponent])
getLifecycleState(host, value)
} catch (e) {

Choose a reason for hiding this comment

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

prefer err for all Error type args
(e is generally used for events)

Copy link
Member Author

Choose a reason for hiding this comment

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

Your right, that's a better convention.

/**
* @return {boolean}
*/
function fetchable (object) {

Choose a reason for hiding this comment

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

suggest rename to isFetchable
though really should be isFetchResponse

@Zolmeister
Copy link

Where are the tests for AsyncGenerator?

@thysultan
Copy link
Member Author

thysultan commented Mar 5, 2018

The AsyncGenerator was a POC after seeing the related RFC, so the test don't yet exist. But i plan to write them and figure out its equivalent server implementation, i figure we can just wait till the generator is done to yield back to the NodeStream renderer.

* @return {object}
*/
function createComponentGenerator (generator) {
return function then (resolve, reject) {
Copy link

@Zolmeister Zolmeister Mar 5, 2018

Choose a reason for hiding this comment

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

shouldn't this be new Promise function(resolve, reject) { }?

Copy link
Member Author

@thysultan thysultan Mar 5, 2018

Choose a reason for hiding this comment

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

The callsite where this is invoked looks like

createElementPromise(createComponentGenerator(element))

Where createElementPromise looks like

return createElement({then: callback})

Since we can work with "thenables" as well we don't need to rely on the Promise constructor for this.

Choose a reason for hiding this comment

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

And if it throws?

@thysultan
Copy link
Member Author

Related to the server-side rendering implementation details of AsyncGenerator.

I wonder if we can actually render intermediate states with intermediate script elements that amend the previous element once the next state has resolve i.e pushing a script element before the next yield to cleanup previous markup that was rendered.

// previous yield
<h1>Loading...</h1>

// intermediate yield to cleanup previous intermediate yield
<script>
var currentScript = document.currentScript
var previousSibling = currentScript.previousSibling
previousSibling.parentNode.removeChild(previousSibling)
currentScript.parentNode.removeChild(currentScript)
</script>

// next yield.. 

@Zolmeister
Copy link

Zolmeister commented Mar 5, 2018

Pretty radical idea, though I'm not sure it's practical.

I think the ideal streaming setup looks something like this:

                                   GET /
client --------------------------------------------------------------------------> server
          <html><head></head><body><div>Loading...</div>           (server queries database here)
client <--------------------------------stream--------------------------------- server
          <script>window.emit('data', {user: {id: 'x'}})</script>  (as database(s) return values)
client <--------------------------------stream--------------------------------- server

let the client update the components as data streams in
<head> would be a promise (blocking)

@thysultan
Copy link
Member Author

thysultan commented Mar 5, 2018

@Zolmeister I don't understand? What does window.emit('data', {user: {id: 'x'}}) do?


...I few thinks to also figure out regarding AsyncGenerators.

  • Testing them, async generators are only supported in NodeJS nightly. (use --harmony?)
  • What yeild ... should return i.e
var value = yield 'Something'
// use previous value

function timeout (callback) {
return setTimeout(function () {
callback(now())
}, 16)

Choose a reason for hiding this comment

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

Maybe 17 ms as 1000 / 60 is 16.7

Choose a reason for hiding this comment

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

16ms = 62.5fps
17ms = 58.8fps

standard refresh rate is 60Hz
so 16ms maximizes frames

Choose a reason for hiding this comment

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

I just thought it was intended as a polyfill for requestAnimationFrame, not something to maximize fps, one could use 1ms to get 1000 fps if the goal was to maximize fps. Although, maybe 16ms is better as there will be delay due to executing JavaScript anyways.

Choose a reason for hiding this comment

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

Above 60fps is worthless because of the 60Hz refresh rate (i.e. why 1ms doesn't make sense), but below 60fps your missing frames

Choose a reason for hiding this comment

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

I don't think above 60Hz is worthless, firstly, because not all people have 60Hz monitor refresh rate, some have higher than that. Also, even if you have only 60Hz refresh rate, the more fps you generate the smoother the animation will feel.

Choose a reason for hiding this comment

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

I was unable to find statistics on monitor refresh rates, so lets estimate:
desktop web market share is ~44% (We, safely, assume mobile devices are 60Hz).
Of desktops ~71% (67% + 4%) run Chrome/Edge, which auto-update, and thus are likely to include requestAnimationFrame.

So, given requestAnimationFrame does not exist, we estimate at most .44 * (1-.71) ~= 13% of users will ever hit this function.
Given a user is using and older browser, I'd guess the likelihood of their computer having a >60Hz monitor is 1%.
So I'd estimate the total users who could ever use more than 60Hz is 0.1%

As for your comment about animation smoothness, it's simply false, because physics. Not just that, it can create artifacts called screen tearing. You are perhaps conflating physics updates with draw updates, which should run at separate rates to optimize animation smoothness / input lag - Game Loop

Copy link

@streamich streamich Mar 6, 2018

Choose a reason for hiding this comment

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

As for your comment about animation smoothness, it's simply false, because physics.

It's actually true, exactly because of physics. Have you ever played computer games?

At 60 fps, your video card is making a new frame every 1/60 of a second or every 16.67 ms. That means every frame that your monitor shows can be delayed up to 16.67 ms.

At 300 fps, your video card is making a new frame every 1/300 of a second or every 3.33 ms. That means every frame that your monitor shows can only be delayed up to 3.33 ms.

Basically, at 300 fps, each frame your monitor shows is more accurate and less delayed, even though it is only showing 60 frames per second.

Plus, if frame rate drops, it will be less noticeable if you are running at 300 fps.

Choose a reason for hiding this comment

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

Another thing is, if you will use that function to create a DRAF, it will actually be 1.3 ms too short.

Choose a reason for hiding this comment

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

I think I understand what you're saying. I now agree that an increase in fps can lead to increased smoothness.

During my research I found this gem: Rendering for "Glass Time", which I found fascinating.

For posterity, the official shim: https://gist.github.com/paulirish/1579671

Choose a reason for hiding this comment

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

The "Glass Time" presentation is indeed interesting. That's why even when you have 60Hz screen its nice to have 300fps when playing computer games.

@Zolmeister
Copy link

Zolmeister commented Mar 5, 2018

@thysultan I was trying to find a way to stream the whole DOM (non-blocking) and update in-place from the stream (i.e. you would see the whole page, perhaps with 10x <loading> elements, all replaced asyncronously)

In retrospect it may not be practical
I like the simplicity of your version

*/
function timeout (callback) {
return setTimeout(function () {
callback(now())

Choose a reason for hiding this comment

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

As much as I like correctness in polyfills, Date.now() is an expensive operation for a value that is never used

if (value.done === true && value.value === undefined)
return !element.active && resolve(getElementDefinition(cache.value))

if ((cache.value = value.value, element.active))
Copy link

@Zolmeister Zolmeister Mar 6, 2018

Choose a reason for hiding this comment

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

It's so silly you even included extra parenthesis. Move the value assignment out of the if expression

instance.props = props
instance.context = context
instance.state = state = instance.state || {}
owner.state = state = Object(owner.state)

Choose a reason for hiding this comment

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

Object() wrapper here seems redundant due to the defaults in the Component class (line 6)

Copy link
Member Author

Choose a reason for hiding this comment

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

This prevents the following from resulting in a non-object state reference

class A extends Component {
constructor() {
this.state = getSomeValueThatMightBeNull()
}
}

if ((cache.value = value.value, element.active))
resolve(getElementDescription(value.value))

then(resolve, reject)

Choose a reason for hiding this comment

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

this call waits on requestAnimationFrame, but shouldn't
Values should resolve as fast as possible (nextTick), e.g. <loading> -> <almostReady> -> <ready> might be able to finish within a single frame

Copy link
Member Author

@thysultan thysultan Mar 6, 2018

Choose a reason for hiding this comment

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

This was intentionally added to support patterns like

async function * Foo ({value}) {
	var step = 0
	
	while (true) {
		if (step)
			yield h('h1', 'Hello ', name)
		else
			yield h('form', {onSubmit: (e) => e.preventDefault(step++)},
				h('input', {placeholder: 'Name?', onInput: ({target}) => name = target.value})
			)
	}
}

Without the need to wrap it a Promise, i.e

async function * Foo ({value}) {
	var step = 0

	while (true) {
		yield new Promise((resolve) => {
			requestAnimationFrame(() => {
				if (step)
					resolve(h('h1', 'Hello ', name))
				else
					resolve(h('form', {onSubmit: (e) => e.preventDefault(step++)},
						h('input', {placeholder: 'Name?', onInput: ({target}) => name = target.value})
					))
			})
		})
	}
}

But i agree this probably shouldn't be at the library level since the pattern you describe is only possible outside of the requestAnimationFrame heuristic.

Choose a reason for hiding this comment

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

Doesn't looping requestAnimationFrame all the time have a big hit on performance?

Choose a reason for hiding this comment

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

Another thing is when Foo props change and it re-render, what happens?

Copy link
Member Author

@thysultan thysultan Mar 6, 2018

Choose a reason for hiding this comment

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

Yes, i think we can remove the requestAnimationFrame heuristic in this case, the example is interesting for animation patterns but still possible without the use of requestAnimationFrame within the implementation details.

Another thing is when Foo props change and it re-render, what happens?

Given the following

async function * Foo ({children, time}) {
	yield wait(h('h1', 'Loading'), time)
	yield h('h1', children)
}

render(h(Foo, {children: 'Hello', time: 2000}), container)
render(h(Foo, {children: 'World', time: 1000}), container)

I think it should be spec'ed to render the last value World vs the last resolved value Hello, what do you think?

Choose a reason for hiding this comment

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

agreed

* @param {function?} callback
*/
function enqueueStateUpdate (element, owner, state, callback) {
if (state) {

Choose a reason for hiding this comment

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

always true because of default {}?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible that it is null/undefined, i.e

this.setState(() => {
if (...)
  // cancel update
  return null

return {...}
})

try {
return element.owner[name].call(element.instance, props, state, context)
getLifecycleState((element.work = SharedWorkIntermediate, element), element.owner[name](props, context))

Choose a reason for hiding this comment

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

I'm going to stop commenting about this style of syntax, but note that I think it's significantly detrimental to comprehension to nest an unused variable assignment into an expression like this

@thysultan
Copy link
Member Author

The previous commit is a POC related to #43

  • Moves a few things around to get this working while at the same type supporting pluggable renderers that can define what a custom-elements is and how to create them.

TODOs:

  • Server-side rendering and hydrating custom-elements(how do we deal with generated names).
  • Tests for custom-elements(jsdom does not support custom elements yet).

Maybe if WICG/webcomponents#566 (comment) lands in the future we can use that instead of the try...catch hack around generating unique custom element names or instead we could just fail – one side of the argument is to avoid naming conflicts related: lit/lit#217

Here is a demo of it in action: https://jsbin.com/deminec/1/edit?html,output

Maybe @treshugart could pitch on what other cases he had to consider with skatejs renderers?

@treshugart
Copy link

Hey @thysultan, what specifically were you looking for feedback on?

@thysultan
Copy link
Member Author

@treshugart

  1. With regards to generating unique custom element names: How does SkateJS deal with selecting the element with CSS.
  2. Any other edge cases(if any) you may have encountered when implemented this for skatejs.

@treshugart
Copy link

treshugart commented Mar 11, 2018

@thysultan, thanks for the clarification.

For posterity, we use http://skatejs.netlify.com/utils/name right now to generate unique names. The algorithm, instead of throwing, simply checks customElements.get() until it finds a unique value based on the incrementing number.

Re point 1

We don't have a built-in way of doing this. I've needed to do this a handful of times and I generally just put a class on the custom element and select via that.

Alternatively, In apps, where you have control over the components, use a static property to store the name. See next example:

import { name, withComponent } from 'skatejs';

class Ce1 extends withComponent() {
  static is = name();
  render() {
    return `Hello, <slot></slot>!`;
  }
}

class Ce2 extends withComponent() {
  static is = name();
  render() {
    return `
      <style>
        ${Ce1.is} {
          display: block;
        }
      </style>
      <${Ce1.is}>World</${Ce1.is}>
    `;
  }
}

customElements.define(Ce1.is, Ce1);
customElements.define(Ce2.is, Ce2);

const ce2 = new Ce2();

I'm not sure there's a use case that would prevent you from doing the former, though. That said, there's other cases for wanting to be able to retrieve the name of a constructor, which is precisely why I proposed WICG/webcomponents#566 (comment). It would also make this solution available via standards because you could do ${customElements.getName(Ce1)} instead of having to define static is.

I'm not sure if I see the standards approach coming to a consensus. There's a solid proposal by Mozilla but others seem to have conflicting views about if it should be implemented for built-ins. It's quite unfortunate because there seems to be a need from the community here.

Re point 2

No real edge cases to point out, other than it makes it hard for consumers to write just HTML, but I think that's okay because this pattern is only super useful for apps or consumers writing apps. One should probably not be pre-defining names for custom elements, and leave that up to the consumer anyways.

There's a whole host of edge cases regarding SSR and context but those are topics better discussed on a case-by-case basis, probably, because it'd be a lot of information and possibly disjointed because I haven't collated my thoughts entirely on this yet.


BTW, cheers for mentioning me. I'm more than happy to share my knowledge whenever :)

@thysultan
Copy link
Member Author

@treshugart Thanks, regarding:

The algorithm, instead of throwing, simply checks customElements.get() until it finds a unique value based on the incrementing number.

Does it do this before trying to invoke the constructor, if so how do you handle custom elements that do explicitly define their own names customElements.define(...) and don't use the .is property heuristic?

custom elements.

- An alternative random string generator that is deterministic.
- Move a few things around.
- Add a createComment API.
- Add a shouldUpdateProps reconciler API.
- Better match React heuristics with regards to Error Boundaries(starts
traversing from one level up the tree).
- Sync handle the pattern of setState in constructor via
`this.setState`.
- Opt to lazy-ly generate the componentStack string for caught error
boundaries.
- Improve internal WeakMap polyfill to use a deterministic random
number as a hash instead of a constant.
- Vanilla invoke custom-element/web-component constructors and leave
the heuristic of generating unique localNames to renderer implementors.
- Add tests for related changes, 100% coverage banzai.
- Improve documentations, in anticipation for a release.

The current heuristics for server rendering AsyncGenerators is to play
it save and only send the last state as the payload, maybe a future
release might render in-betweens if rendering intermediate values
proves to be a better UX.

The current heuristics for ErrorBoundaries that catch errors thrown in
their tree when server-side rendering with renderToNodeStream is to
play it safe and not render the thrown component/tree.

TODO:

- Performance tests.
- Third party breakage tests(this release is intended to only break the
heuristics around ErrorBoundaries that did not match React 1-to-1).
- Figure out a codename for the release ;)
- reconciler: rename `getProps` to `getInitialProps`.
- reconciler: change `shouldUpdateProps` to `getUpdatedProps`.
- reconciler: expose `willUnmount` method.
- reconciler: expose `getListener` method.
- resolve `defaultProps` from `createClass` components.
- improve some function names.
- improve UMD wrapper.
- improve bundler test.
- improve typescript definitions.
- invoke `componentWillUnmount` depth first to allow children the ability to subscribe to parent unmount animations.

TODO:

performance tests.
- improve UMD wrapper.
@thysultan
Copy link
Member Author

First alpha release out – https://unpkg.com/dio.js@9.0.0-alpha/

- Improve both context implementation.
- Improve unmount implementation.
- Pass host element to custom reconcilers willUnmount method.
- Handle circular references in props.
- Improve jsdocs.
- Revert to previous context implementation that does not rely on the
old context API to ensure both API’s can co-exist, related:
facebook/react#12551 (comment)

- Support `defaultValue` on select elements.

- Fix getChildContext’s update phase invocation to happen after state
has updated but before “render”.
- Error: Only force unmount if an ErrorBoundary has not caught the error this is motivated by how well this will work with future work on a Suspense API and the use cases that @Zolmeister previously mentioned in 8.0.0.
- Tests: Move all ref related tests to a dedicated ref spec file.
- Ref: Improve ref implementation.
- Render: only set defaultValue for the select elements on initial mount.
- Reconciler: Expose a signature argument to `setProps` that reconcilers can use to determine the phase at which the method is being invoked in.
- Reconciler: Pass the container as an argument to `getPortal`.
- Improve select defaultValue implementation.
- Improve Children.toArray implementation.
- Support multiple event handlers on a single event.
- Add Todo and Custom Elements examples to docs.
@thysultan
Copy link
Member Author

I think this is ready for a merged and release if nothing else comes up.

@rosenfeld
Copy link

The ErrorBoundary feature seems to be broken in the 9.0 branch. You can give it a try in that project I shared with you.

@thysultan
Copy link
Member Author

thysultan commented Apr 10, 2018

@rosenfeld What exactly is broken? i can't seem to reproduce the previous issue on my end, are you using the latest release candidate?

Edit: Yarn might be downloading 9.0.0-rc instead of 9.0.0-rc.1. At least that's what i get on a fresh clone.

@rosenfeld
Copy link

With Dio.js 8.2.4, when I throw an error in render I see the ErrorBoundary working correctly and displaying "Sorry, but something bad happened and we have been notified about it.".

With 9.0.0-rc.1 this message is not shown. I just pushed the changes to dio-9 upgrading to latest RC. Didn't do that before because I thought you'd run "yarn add --dev dio.js@next" yourself, but just to be sure, the latest version of the "dio-9" branch should demonstrate the issue. If you run "yarn add --dev dio.js" and run webpack again you should notice the different behavior, while the behavior in version 8 is the expected one regarding the ErrorBoundary component.

- Patch ErrorBoundary bug with interleaved corrupted children.
- Improve documentation.
@thysultan
Copy link
Member Author

@rosenfeld I was able to find a isolated reproduction to add to the tests. This should work now.

@rosenfeld
Copy link

That's awesome, thanks! I can confirm it's working fine now :)

- Improve documentation.
@thysultan thysultan merged commit 80d9281 into master Apr 11, 2018
@thysultan thysultan deleted the 9.0.0 branch April 24, 2018 20:22
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

6 participants