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

Hyperapp V2 #726

Open
wants to merge 63 commits into
base: master
from

Conversation

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jul 9, 2018

Summary

馃憢 V2 is the pet name for the next version (2.0.0) of Hyperapp. Be sure to subscribe to this PR if you want to be informed of the progress towards the release. I'm still open to feedback and ideas, so share your feedback freely!

To learn more about the upcoming architecture changes check the issue list below.

Status Progress

  • Docs
  • Tests
  • Core
    • Implement Action API #750 #751
    • Implement the Effect API #750
      • Add built-in batching for effects
    • Implement Subscriptions API #752
    • Optimize defer function (setTimeout to use faster Promise.resolve)
  • VDOM
    • Improve performance #499
    • Lazy lists #721
    • Dynamic components #760
    • Add built-in classcat-like support for className attribute
  • Typings

Try it!

npm i hyperapp@alpha
V2

@jorgebucaran jorgebucaran added this to the 2.0.0 milestone Jul 9, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 9, 2018

Codecov Report

Merging #726 into master will decrease coverage by 83.69%.
The diff coverage is 16.3%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #726      +/-   ##
=========================================
- Coverage     100%   16.3%   -83.7%     
=========================================
  Files           1       1              
  Lines         166     233      +67     
  Branches       52      69      +17     
=========================================
- Hits          166      38     -128     
- Misses          0     130     +130     
- Partials        0      65      +65
Impacted Files Coverage 螖
src/index.js 16.3% <16.3%> (-83.7%) 猬囷笍

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 ebe173a...df89651. Read the comment docs.

@jorgebucaran jorgebucaran referenced this pull request Jul 9, 2018

Closed

RFC: Hyperapp 2.0 #672

@Swizz Swizz self-requested a review Jul 9, 2018

@mindplay-dk
Copy link

mindplay-dk left a comment

Typescript definitions appear to be out of date? I copy/pasted the example from the README, and it fails because the .d.ts has no exported member app. (looks like a lot of the types were deleted?)

@mindplay-dk

This comment has been minimized.

Copy link

mindplay-dk commented Jul 9, 2018

I also noticed there's a block of 3 commented-out functions left behind in the source.

@SteveALee

This comment has been minimized.

Copy link

SteveALee commented Jul 9, 2018

REAME seems unchanged. As I've not been part of the V2.0 discussion I need a way get context and understand the goals. At least I prefer that before reading code.

@okwolf

This comment has been minimized.

Copy link
Contributor

okwolf commented Jul 9, 2018

Wow, still single file source. Is that the plan to keep that going forward?

@jorgebucaran

This comment has been minimized.

Copy link
Owner Author

jorgebucaran commented Jul 9, 2018

@okwolf For starters, yeah. I had everything in its own file until the last moment.

@lukejacksonn

This comment has been minimized.

Copy link
Contributor

lukejacksonn commented Jul 10, 2018

I was really looking forward to seeing it all split up.. I feel the one file things obfuscates different functions reliance on the various globals. You can't get away with that when everything is split up because you have to pass a function everything it needs. Still nice code though. Much more comprehensible that the last one, great job.

@jorgebucaran

This comment has been minimized.

Copy link
Owner Author

jorgebucaran commented Jul 10, 2018

I feel the one file things obfuscates different functions reliance on the various globals.

We'd just have a constants.js file and export the vnode types and every function required by any given function would need to import it at the top of the file.

@lukejacksonn

This comment has been minimized.

Copy link
Contributor

lukejacksonn commented Jul 10, 2018

Yeh.. I know how it would work. I was looking forward to that structure, that is all. Thought it would make it more clear to people what is going on and what relies on what. It is ok though, I will do it myself sometime as a learning exercise.

@jorgebucaran

This comment has been minimized.

Copy link
Owner Author

jorgebucaran commented Jul 10, 2018

@lukejacksonn Do you think it would be better to split up the code then?

@jorgebucaran

This comment has been minimized.

Copy link
Owner Author

jorgebucaran commented Jul 10, 2018

@lukejacksonn Yeh.. I know how it would work.

For sure, but then I misunderstood this:

You can't get away with that when everything is split up because you have to pass a function everything it needs


My point is, while everything is crammed in one file, every function is independent and already expects as input everything it needs to work on and it doesn't rely on stuff which is in the global scope, other than the stuff you'd import to the top-level scope of the module in which the function is declared if it was in a file.

In other words, the amount of refactoring needed to put everything into files is minimal, that's what I meant. Sorry, if I said something confusing! 馃槃

@lukejacksonn

This comment has been minimized.

Copy link
Contributor

lukejacksonn commented Jul 10, 2018

You didn't say anything confusing 馃槄 and I can see that it would easily refactor. I love the single file thing too, but I am starting to wonder if it would be easier to grok in individual files.

@Swizz

This comment has been minimized.

Copy link
Contributor

Swizz commented Jul 10, 2018

I love the one function per file too.
But it could be confusing too, you will need to dive into an src folder with multitude of files, to find the entrypoint, mostly index then to jump from file to file to catch all the function calls.

Here, it is like reading a story, you learn how the function works, then, when it is later called, you already know the function and how it works.

I was for the individual function files, but now I am thinking about it, I am not sure anymore.

@Swizz
Copy link
Contributor

Swizz left a comment

General thoughts :

  • Still not sure about individual files, but I like the way it is in one file.
  • Would love the use of const for constantes but it is not ES3/5 compliant 馃
  • Did not read so much the patch part (I am not a vdom scientist)
for (length = node.length; length--; ) {
rest.push(node[length])
var EMPTY_OBJECT = {}
var EMPTY_ARRAY = []

This comment has been minimized.

@Swizz

Swizz Jul 10, 2018

Contributor

馃

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 10, 2018

Author Owner

A small optimization, mainly used for text vnodes that don't have children, but still use the same VNode shape.

return target
}

var resolved = typeof Promise === "function" && Promise.resolve()

This comment has been minimized.

@Swizz

Swizz Jul 10, 2018

Contributor

So now Promises are used in core ? (if supported)

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 10, 2018

Author Owner

Nope, this is just a trick to call your callback as quickly as possible.

This comment has been minimized.

@kristianmandrup

kristianmandrup Jan 13, 2019

Why still use var in 2019? Please use const. No need to be ES3/5 compliant

This comment has been minimized.

@jorgebucaran

jorgebucaran Jan 13, 2019

Author Owner

@kristianmandrup Check out CONTRIBUTION.md鈥攊t's written there. Long story short: because we're not compiling Hyperapp, it's handwritten.

var oldChildren = oldNode.children
var children = node.children
return typeof name === "function"
? name(props, (props.children = children))

This comment has been minimized.

@Swizz

Swizz Jul 10, 2018

Contributor

A Component will received children property and children argument ?

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 10, 2018

Author Owner

Good catch. It's an implementation detail. If you need children for some reason, use props.children. I am still open to change this.

This comment has been minimized.

@jorgebucaran

This comment has been minimized.

@frenzzy

frenzzy Jul 10, 2018

Contributor

props.children definitely cooler then the second argument, see pros and cons here: #569
I am worried only about performance and memoization:
https://github.com/hyperapp/hyperapp/blob/2b3d85625321038ed2369ddb4ecc320a9e20984e/src/index.js#L458

If everything is ok with props.children would be nice to remove children from the second argument IMO.

This comment has been minimized.

@frenzzy

frenzzy Jul 13, 2018

Contributor
const Component = (props) => <div>{props.children}</div>

app({
  init: { children: 'Hello' },
  view: (state) => (
    <main>
      <Component {...state}>World</Component>
      <div>{state.children}</div>
    </main>
  ),
  container: document.body,
})

Can you guess what this code will render without opening the demo?

  1. <main><div>World</div><div>Hello</div></main>
  2. <main><div>Hello</div><div>World</div></main>
  3. <main><div>Hello</div><div>Hello</div></main>
  4. <main><div>World</div><div>World</div></main>
  5. throws an error

Demo: https://codepen.io/frenzzy/pen/Zjbqpq/left/?editors=0010

This comment has been minimized.

@Swizz

Swizz Jul 14, 2018

Contributor

馃憣

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 15, 2018

Author Owner

Any alternatives? Why can preact get away with the same hack? 馃

This comment has been minimized.

@frenzzy

frenzzy Jul 16, 2018

Contributor

Purity forever! Say NO to mutations :D

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 16, 2018

Author Owner

Okay, so to clarify, we're back to (props, children)?

This comment has been minimized.

@frenzzy

frenzzy Jul 16, 2018

Contributor

Yes!

  • no mutations
  • no pitfalls for children key
  • simple memoization with ===
}
var dispatch = function(obj, data) {
typeof obj === "function"
? dispatch(obj(state, data))

This comment has been minimized.

@Swizz

Swizz Jul 10, 2018

Contributor

Clever.

But the whole codeblock is pretty unreadable 馃

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 10, 2018

Author Owner

I agree. I'm working on a new implementation. 馃憤

This comment has been minimized.

@TheNando

TheNando Jul 10, 2018

Chaining ternaries is not so bad if you change the formatting a little. I know this is unorthodox, but it definitely helps with clarity.

typeof obj === "function" ?
    dispatch(obj(state, data))
: isArray(obj) ?
    obj[1].effect(obj[1].props, dispatch, setState(obj[0]))
: obj != null && typeof obj.effect === "function" ?
    dispatch([state, obj])
: setState(obj)

This comment has been minimized.

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 11, 2018

Author Owner

No can do, we're using prettier.

}

dispatch(props.init)
}

This comment has been minimized.

@Swizz

Swizz Jul 10, 2018

Contributor
return dispatch

?

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 10, 2018

Author Owner

Yes, I'm still not sure if I should return it or not.

This comment has been minimized.

@dangerousdan

dangerousdan Jul 10, 2018

dispatch doesn't return anything, so it will be undefined either way, right?

This comment has been minimized.

@Swizz

Swizz Jul 10, 2018

Contributor

It is about returning the dispatch function itself not the dispatch(props.init) result.

Just commented on this line as this is the last line of the app function.

This comment has been minimized.

@frenzzy

frenzzy Jul 10, 2018

Contributor

How to get the state of your application at specific moment of time? For example for HMR

This comment has been minimized.

@okwolf

okwolf Jul 11, 2018

Contributor

So should interop be done with subscribe then?

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 11, 2018

Author Owner

@frenzzy There is currently no way other than using subscriptions. But I am open to exploring alternatives if need be.

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 11, 2018

Author Owner

@okwolf Bingo.

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 11, 2018

Author Owner

Like Elm. But still, if we can think of an alternative, e.g., app.getState(), I am happy to discuss it.

This comment has been minimized.

@okwolf

okwolf Jul 11, 2018

Contributor

@jorgebucaran we should start by trying to build things that need interop and find where this approach gets ugly.

}
*/

export function app(props) {

This comment has been minimized.

@Swizz

Swizz Jul 10, 2018

Contributor

How about instead of inline exports, using the following block at the top or the bottom of the file

export { h, app }

To make clear that h and app will be exported.

I am using this pattern and I found it verry clever. (I learned it from Erlang).

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 10, 2018

Author Owner

I could like that. Let me do some research.

This comment has been minimized.

@frenzzy

frenzzy Jul 10, 2018

Contributor

It is definitely more clear when all exports goes near to each other, but I found out that for example webpack generate more noise when you export a name instead of an expression.

var dispatch = function(obj, data) {
typeof obj === "function"
? dispatch(obj(state, data))
: Array.isArray(obj)

This comment has been minimized.

@dangerousdan

dangerousdan Jul 10, 2018

Should this be isArray(obj) for the sake of consistency?

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 10, 2018

Author Owner

Yup! Good catch and thanks. I'll push updated code soon.

var length = arguments.length
var DEFAULT = 0
var RECYCLED_NODE = 1
var TEXT_NODE = 2

This comment has been minimized.

@dangerousdan

dangerousdan Jul 10, 2018

This seems misleading.

https://github.com/hyperapp/hyperapp/blob/2b3d85625321038ed2369ddb4ecc320a9e20984e/src/index.js#L420

Should your internal constants match the JS spec, or should this be renamed to avoid confusion?

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 10, 2018

Author Owner

@dangerousdan Haha you do have a point. I'll fix it.

@jorgebucaran

This comment has been minimized.

Copy link
Owner Author

jorgebucaran commented Jul 10, 2018

@atomiks #726 (comment)

I considered something like it, but I feel now that a mixed approach would break the balance. It's all or nothing. Split the code or not, but not in between is what I mean :)

eventProxy,
isSvg
) {
if (name === "key") {

This comment has been minimized.

@Mytrill

Mytrill Jul 10, 2018

Contributor

Could also be

if(name === "style") {
  ...
} else if (name !== "key") {
  ...
}

To avoid an empty if

This comment has been minimized.

@TheNando

TheNando Jul 10, 2018

I'm a fan of the "exit early" pattern

if (name === "key") return

// Do other ifs

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 11, 2018

Author Owner

I wonder if this is just personal taste or there's something to this. I mean, my brain finds it a lot easier to think of this as: (if something is true; do nothing), instead of (if not something; do something). Not always, though.

: document.createElement(node.name)

var props = node.props
if (props.onCreate) {

This comment has been minimized.

@okwolf

okwolf Jul 11, 2018

Contributor

So lifecycle events are still supported and use camelCase now? 馃

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 11, 2018

Author Owner

I gave in to camelCase :)

...lifecycle events

They're in this branch, for now. I still haven't decided whether to remove them or not. I know we don't need them, but having them can be useful. You can help me decide by adding your feedback to #717.

This comment has been minimized.

@SkaterDad

SkaterDad Jul 19, 2018

Contributor

but having them can be useful.

I'm glad the hooks are still there so I don't yet have to use custom elements and allow this in my codebase 馃あ

@okwolf

This comment has been minimized.

Copy link
Contributor

okwolf commented Jul 11, 2018

Will hyperapp@2.0.0-beta.0 be published from this branch or not until merged to master?

@jorgebucaran

This comment has been minimized.

Copy link
Owner Author

jorgebucaran commented Jul 11, 2018

@okwolf Published from master after it's merged (with docs). Future V1 releases will be published from the V1 branch, though.

}

dispatch(props.init)

This comment has been minimized.

@znk

znk Jul 11, 2018

dispatch(props.init || {})

Providing init with an empty state permits to omit it in case of concise example or stateless app. We can write like:

app({
  view: () => (
    <div>
      Lorem ipsum dolor si amet
    </div>
  ),
  container: document.body
})

instead of:

app({
  init: () => ({ }),
  view: () => (
    <div>
      Lorem ipsum dolor si amet
    </div>
  ),
  container: document.body
})

This comment has been minimized.

@Swizz

Swizz Jul 11, 2018

Contributor

props.init is a function, so it should be

dispatch(props.init || function() {})

or in a more concise way

dispatch(props.init || Function())

This comment has been minimized.

@Swizz

Swizz Jul 11, 2018

Contributor

Nevermind, dispatch works with objects too.

(part of the unreadable ternary 馃檮 )

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 12, 2018

Author Owner

Yup.

@kumarabhishek
Copy link

kumarabhishek left a comment

My major comment is to use as much ES6 as possible to make code simpler, smaller and performant.

"rollup": "^0.57.1",
"typescript": "2.8.1",
"uglify-js": "3.3.16"
"babel-preset-env": "^1.7.0",

This comment has been minimized.

@kumarabhishek

kumarabhishek Jul 11, 2018

Can we use latest babel 7 packages ?

I have recently tried using rollup and parceljs for bundling one of my library and finally i ennded up using webpack 4.15.x because i wanted to use latest babel packages as descibed below:

    "@babel/cli": "^7.0.0-beta.51",
    "@babel/core": "^7.0.0-beta.51",
    "@babel/plugin-proposal-object-rest-spread": "^7.0.0-beta.51",
    "@babel/plugin-syntax-dynamic-import": "^7.0.0-beta.51",
    "@babel/preset-env": "^7.0.0-beta.51",
    "@babel/preset-react": "^7.0.0-beta.51",

If rollup is finalised for V2 then ignore this comment.

This comment has been minimized.

@ggassmann

ggassmann Jul 19, 2018

@jorgebucaran I would not anticipate babel 7 leaving beta soon. They have many open issues.

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 19, 2018

Author Owner

Thanks for clarifying that! @ggassmann

This comment has been minimized.

if (state !== newState) {
state = newState

if (!updateInProgress) {

This comment has been minimized.

@kumarabhishek

kumarabhishek Jul 11, 2018

What will happen if updateInProgress is true ? Are we ignoring all setState() call till updateInProgress is true ? Just a wild thought of saving it to tempState(keep overwriting till updateInProgress is true) and apply the latest value of tempState once updateInProgress becomes false.

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 12, 2018

Author Owner

@k-murakami0609 Are we ignoring all setState() call till updateInProgress is true ?

Nope! We're avoiding patching, that's it. In other words, state updates are pretty much immediate.

cb(element, done)
} else {
done()
export var h = function(name, props) {

This comment has been minimized.

@kumarabhishek

kumarabhishek Jul 11, 2018

As we are exporting h object here so this source file becomes ES6 module. So why then we are not using all power of ES6. To name a few, using const, Object.assign, spread operator, etc which will make code more readable, smaller (not larger at least) and simple without adding any burden to performance. Making methods will easily become one liners.

This comment has been minimized.

@TheNando

TheNando Jul 11, 2018

I don't think this file gets transpiled. Rollup supports specifying UMD, which is how this project is configured, which is probably where the export support comes from.
https://github.com/hyperapp/hyperapp/blob/2b3d85625321038ed2369ddb4ecc320a9e20984e/package.json#L27

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 12, 2018

Author Owner

@kumarabhishek https://github.com/hyperapp/hyperapp/blob/master/CONTRIBUTING.md#style


FYI: We're not losing any performance from not using more ES6 idioms.

But that could change in the future if we start using things like Maps or WeakMaps and they become faster than regular objects? 馃

var listen = function(sub, oldSub, dispatch) {
if (isArray(sub) || isArray(oldSub)) {
var out = []
var subs = isArray(sub) ? sub : [subs]

This comment has been minimized.

@kumarabhishek

kumarabhishek Jul 11, 2018

Here you are doing duplicate isArray check for sub which is already done in if condition. So the tradeoff is having to variables to store result of isArray for sub and oldSub versus calling isArray two times. I will consider more variables as cheaper than more function call in runtime. My idea is something like this:

const isArraySub = isArray(sub)
const isArrayOldSub = isArray(oldSub)
if (isArraySub || isArrayOldSub) {
  let out = []
  let subs = isArraySub ? sub : [subs];
  ...
}

This comment has been minimized.

@TheNando

TheNando Jul 11, 2018

Well, that won't work with let, only with var. Var hoists the definition to the top of the scope, so when it's referenced at [subs] it returns undefined. But with let it will throw a ReferenceError, since it is self referencing and won't exist until the line has executed.

I actually think this is a typo and he was intending to ensure that sub is an array. If that's the case, you can just do:

var subs = [].concat(sub)

Which will ensure that subs ends up an array and will work if sub is already an array or just a value.

This comment has been minimized.

@Swizz

Swizz Jul 12, 2018

Contributor

@TheNando I have already made this suggestions one month ago on Slack
see https://hyperapp.slack.com/archives/C5D86NQ3X/p1529772214000096?thread_ts=1529772214.000096&cid=C5D86NQ3X

If @jorgebucaran made the decision to do not use this implementation, he should have some good reasons.

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 12, 2018

Author Owner

Thanks, @Swizz. Basically [].concat is a lot slower.

This comment has been minimized.

@TheNando

TheNando Jul 13, 2018

@jorgebucaran But : [subs] supposed to be : [sub], right?

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 13, 2018

Author Owner

@TheNando Yes, totally, that's a bug! Fixing it now :)

var subscribe = props.subscribe
var subscriptions = []
var container = props.container
var element = container && container.children[0]

This comment has been minimized.

@kumarabhishek

kumarabhishek Jul 11, 2018

Is container.children always defined and thus container.children[0] is always valid ?

This comment has been minimized.

@jorgebucaran

jorgebucaran Jul 12, 2018

Author Owner

@kumarabhishek Only if container exists!

container && ...
updateAttribute(element, name, attributes[name], null, isSvg)
}
}
var createTextVNode = function(text, element) {

This comment has been minimized.

@kumarabhishek

kumarabhishek Jul 11, 2018

If you use ES6 default parameter and organise params like below then createTextVNode is not needed at all:

var createVNode = function(name, element, props={}, children=[],  key=null, type=TEXT_NODE) {
 ...
};

This comment has been minimized.

@okwolf

okwolf Aug 27, 2018

Contributor

Please read this regarding which ES spec we follow.

}
}

var getKey = function(node) {

This comment has been minimized.

@kumarabhishek

kumarabhishek Jul 11, 2018

This method will not be needed if we ensure that node.key is by default null. This can be done using default parameter in functions related to node.

) {
for (var name in merge(lastProps, nextProps)) {
if (
(name === "value" || name === "checked"

This comment has been minimized.

@kumarabhishek

kumarabhishek Jul 11, 2018

Just a matter of taste, i like single quotes for strings in JS and double quotes inside JSX/HTML.

This comment has been minimized.

@joseluisq

joseluisq Jul 11, 2018

It's not bad suggestion, in fact I prefer single quotes for strings too.
Even it's supported by prettier https://prettier.io/docs/en/options.html#quotes

This comment has been minimized.

@joseluisq

joseluisq Jul 11, 2018

But in that case, It's Hyperapp @jorgebucaran preference 馃槄

@TheNando

This comment has been minimized.

Copy link

TheNando commented Jul 11, 2018

@kumarabhishek Regarding using ES6 over ES5...

My major comment is to use as much ES6 as possible to make code simpler, smaller and performant.

Simpler, maybe. Smaller, probably not after transpilation. Definitely not more performant.

Ultimately, it's per-browser implementation, but in most cases, ES6 is less performant. Declaring arrow functions are a little slower in Chrome and much slower FF. Destructuring is much slower across the board. Classes are slower except in FF. The new loops, generators, map/set, Object.assign, spread...pretty much all slower than es5 implementations.

http://incaseofstairs.com/six-speed/

jorgebucaran added some commits Mar 8, 2019

}
state = newState
}

var dispatch = function(obj, props) {
if (obj == null) {

This comment has been minimized.

@SkaterDad

SkaterDad Mar 12, 2019

Contributor

Was this removed purely for size?

I was use/abusing that null check to avoid needing to wrap dispatch calls in conditionals sometimes.

EDIT: I'm fine with this removal if needed, since dispatch is only called in effects/subs, but it's definitely something we will need to document well. Otherwise you'll end up setting state to null/undefined accidentally.

This comment has been minimized.

@jorgebucaran

jorgebucaran Mar 12, 2019

Author Owner

@SkaterDad

Was this removed purely for size?

Partly, yes. The other reason is that I don't think there is a use case for returning null or undefined from an action. Is there?

I was use/abusing that null check to avoid needing to wrap dispatch calls in conditionals sometimes.

Show me an example, please. 馃檹

This comment has been minimized.

@SkaterDad

SkaterDad Mar 12, 2019

Contributor

Before the recent commits, I could do this:

function myEffect(props, dispatch) {
  // props.init could be undefined.
  // but dispatch was checking for `obj == null`, so it was okay
  dispatch(props.init);

 // do other stuff
  setTimeout(() => {
    dispatch(props.action);
  }, 100)
}

Now my effect code will have to to wrap optional props in an if...then statement:

function myEffect(props, dispatch) {
  // props.init could be undefined.
  // If we don't check for truthiness here, dispatch will end up doing `setState(undefined)`
  if (props.init) {
    dispatch(props.init);
  }

 // do other stuff
  setTimeout(() => {
    dispatch(props.action);
  }, 100)
}

Again, not a huge deal, since it only affects subscriptions/effects implementations. Just want to capture this in the docs.

This comment has been minimized.

@jorgebucaran

jorgebucaran Mar 12, 2019

Author Owner

@SkaterDad Ah, I see. Thanks. Convenient, but maybe not so if you needed to do something when props.init is undefined instead. In that case, we'll end up with two checks: Hyperapp's and yours. I'll note this on the documentation at some point. 馃憤

@frenzzy

This comment has been minimized.

Copy link
Contributor

frenzzy commented on src/index.js in c4a00e7 Mar 12, 2019

no more support for draggable, spellcheck and translate attributes? #628 #629

This comment has been minimized.

Copy link
Owner Author

jorgebucaran replied Mar 14, 2019

@frenzzy You mean SSR support. I'm not sure what I'm going to do about these bad apples. Maybe I can find a universal solution, but I decided to remove the built-in lookup until then.

This comment has been minimized.

Copy link
Contributor

frenzzy replied Mar 14, 2019

You: hyperapp please render <div draggable="false" />
Hyperapp: no problem, done <div draggable="true" />
You: WTF?

This is a bug.
Of cause you can say there are a "work around" for this issue, just use boolean.
Ok well, it is not obvious from user point of view (different people come up with this issue previously), it does not follow specification and of course it is not possible to do SSR for such attribute because boolean false means does not render attribute at all and only "false" as a string means what it should.

This comment has been minimized.

Copy link
Owner Author

jorgebucaran replied Mar 15, 2019

@frenzzy As I said, maybe I can find a universal solution in the future. Not a bug, though. Just a quirk of the DOM.

This comment has been minimized.

Copy link
Contributor

frenzzy replied Mar 15, 2019

Why you can't leave current solution until you find better one?
Otherwise you should add to V2 changelog something like:

No more support for "false" value in draggable, spellcheck and translate attributes. If you are using them, change string "false" to boolean false, otherwise Hyperapp V2 will render them as true. This is a breaking change, so please don't forget to update these attributes during migration from V1 to V2.

This comment has been minimized.

Copy link
Owner Author

jorgebucaran replied Mar 15, 2019

@frenzzy Because maybe there is no universal solution. If I publish V2 with support for this, I'll need to keep it until V3? If I go without it, I have a choice.

This comment has been minimized.

Copy link
Contributor

frenzzy replied Mar 15, 2019

But this is not a feature, this is a bug (no bug in V1, but in V2), ohh.. okay

This comment has been minimized.

Copy link
Owner Author

jorgebucaran replied Mar 15, 2019

You can call it whatever you want. I'm not sure if this is a bug. I'd need to think about it.

I don't know if I will support this. If I find a universal solution, I definitely will. If I don't, I have two choices: go back to what V1 was doing or declare no support. That is most likely to happen after V2 is out, though.

@frenzzy

This comment has been minimized.

Copy link
Contributor

frenzzy commented on src/index.js in c4a00e7 Mar 12, 2019

no more support for xlink:href attribute? #668 #645 #211

This comment has been minimized.

Copy link
Owner Author

jorgebucaran replied Mar 15, 2019

@frenzzy The only offenders are Safari and iOS. But let me see.

@SkaterDad

This comment has been minimized.

Copy link
Contributor

SkaterDad commented Mar 12, 2019

SandBox/CodePen?

Here's a sandbox demonstrating how SVG sprites break when you don't check for xlink:href.
https://codesandbox.io/s/186m022m57

Lately I've come to prefer SVG sprites for icon sets because of how small they are, so it would be great to have this supported.

@SkaterDad

This comment has been minimized.

Copy link
Contributor

SkaterDad commented Mar 12, 2019

Found another broken thing after the latest updates. If a subscription is removed from the array completely, not just made false-y, the cancel function doesn't get called anymore. The example is a little contrived, but I was working on something which requires the previous behavior.

https://codesandbox.io/s/oo98pkp3wq

@mrozbarry

This comment has been minimized.

Copy link

mrozbarry commented Mar 12, 2019

@SkaterDad Yup, looks like subscriptions are currently implemented to be toggled, not fully removed; https://github.com/jorgebucaran/hyperapp/blob/V2/src/index.js#L50 . That seems wrong, especially if you are building subscriptions from a state array map.

@SkaterDad

This comment has been minimized.

Copy link
Contributor

SkaterDad commented Mar 12, 2019

@mrozbarry Glad I'm not the only one to see it.

Back to alpha.8 for now.

@jorgebucaran

This comment has been minimized.

Copy link
Owner Author

jorgebucaran commented Mar 13, 2019

@SkaterDad Here's a sandbox demonstrating how SVG sprites break when you don't check for xlink:href.

The only main offenders are Safari and iOS right now.

<use href="https://codesandbox.io/s/k93mkj3j7v"/>

...works on IE11, Edge, Chrome, and Firefox. I might add it back, though. Let me see.

@jorgebucaran

This comment has been minimized.

Copy link
Owner Author

jorgebucaran commented Mar 13, 2019

@SkaterDad If a subscription is removed from the array completely, not just made false-y, the cancel function doesn't get called anymore.

Not difficult to fix, but to be sure, did you know this works?

return [some, subs, before, state.enable && [[mySub]]]

@mrozbarry That seems wrong, especially if you are building subscriptions from a state array map.

Ah, you may be onto something. Any idea of how that might look?

@SkaterDad

This comment has been minimized.

Copy link
Contributor

SkaterDad commented Mar 13, 2019

@jorgebucaran Yep, I know that works, but I've been building something which generates the array of subs based on state.

@jorgebucaran

This comment has been minimized.

Copy link
Owner Author

jorgebucaran commented Mar 13, 2019

@SkaterDad How does that work or look? If you can paste the relevant code here, it would be helpful.

You should be careful about how you generate subscriptions. Array indices are to subscriptions what keys are to nodes in the VDOM. If you reduce or filter the subscriptions array, it grows or shrinks, and we end up resetting some subscriptions with didn't have to.

jorgebucaran added some commits Mar 13, 2019

@SkaterDad

This comment has been minimized.

Copy link
Contributor

SkaterDad commented Mar 13, 2019

@SkaterDad How does that work or look? If you can paste the relevant code here, it would be helpful.

You should be careful about how you generate subscriptions. Array indices are to subscriptions what keys are to nodes in the VDOM. If you reduce or filter the subscriptions array, it grows or shrinks, and we end up resetting some subscriptions with didn't have to.

Here's some psuedocode, but it illustrates what I'm doing. Using the indices as keys works perfectly fine, as I'm only adding/removing from the end of the array dynamically. I'll be careful not to add/remove before the dynamic ones.

subscriptions: function(state) {
  return [
    StaticSub1,
    StaticSub2,
    state.cond && OptionalSub1,
    ...computedSubs(state)
  ]
}

Will give the new alpha a try today 馃憤

@mrozbarry

This comment has been minimized.

Copy link

mrozbarry commented Mar 13, 2019

The latest commits might fix it, @jorgebucaran I'll take a look over my lunch break.

jorgebucaran added some commits Mar 13, 2019

@SkaterDad

This comment has been minimized.

Copy link
Contributor

SkaterDad commented Mar 13, 2019

@jorgebucaran The new commits (alpha-10) are working great. Thanks for the quick fix 馃

element.removeAttribute(name)
}
} else {
if (name[0] === "o" && name[1] === "n") {

This comment has been minimized.

@diontools

diontools Mar 15, 2019

It looks like we can #674 in alpha.10.

This comment has been minimized.

@jorgebucaran

jorgebucaran Mar 15, 2019

Author Owner

@diontools Good catch!

jorgebucaran added some commits Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.