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

wip: introduce poc cli #45

Closed
wants to merge 18 commits into from
Closed

wip: introduce poc cli #45

wants to merge 18 commits into from

Conversation

marionebl
Copy link
Contributor

@marionebl marionebl commented Jul 31, 2016


Old version

This shows a possible way forward for #43:

bankai-basic

New stuff

  • Introduce bankai start
  • Automagical js, css embedding
  • choo rehydration support
  • html, css, js flag namespaces for the corresponding bankai methods
  • HMR 🎉

Changed stuff

  • development is assumed as default environment, that is pretty breaking

Things to consider

  • browserify opts.debug should be true if env !== 'production'?
  • Assumptions around entry module are pretty hefty? Should this export a function or a choo app?
  • Should the rehydration stuff detect a choo app and do its magic only then?

Notes

  • Jumping through some hoops because the target Node.js versions are not clear. Is 0.12 still supported as https://github.com/yoshuawuyts/bankai/blob/master/.travis.yml#L2 implies?
  • Is the current API untouchable? Changing to (opts) => {css, html, js} would make things considerably easier and would allow to get rid of most of the state stuff. Remains could be put into a state container
  • Rehydrating choo is a bit clumsy, using replaceNode obviously is sub optimal. It would help a big deal if choo retained the id it hydrates on to support consecutive app.start(selector) calls

ToDo:

  • Full rehydration, pass initial state via script[type=application/json]
  • livereload/hmr? server for development environments, trigger change on watchify updates
  • Kill watchify when server is closed. Bug surfacing after changes in bc758da
  • Documentation
  • Cache busting
  • Squash all the things
  • Tests?

@yoshuawuyts
Copy link
Member

Oh man, you're killing it! Re: some of the points you're making:


browserify opts.debug should be true if env !== 'production'?

I think it'd be wise to always compile with sourcemaps, but split them out using exorcist. A second pass through uglify might mess with things though.


Assumptions around entry module are pretty hefty? Should this export a function or a choo app?

Hmh, so I think it'd be best to make as few assumptions as possible about the entry point. For choo specifically, we can add live reloading once yoshuawuyts/barracks#59 lands, making it possible to just be a app.use() style plugin (I'll land it so we can use it here). Would probably be cool if we could split out some of the HMR logic to be generic so it can be reused; and then creating a thin choo specific wrapper.


Should the rehydration stuff detect a choo app and do its magic only then?

I was thinking: perhaps we should make enabling the HMR stuff to explicit? I think having it on in development for a CLI would make sense, toggled in the API.


Jumping through some hoops because the target Node.js versions are not clear. Is 0.12 still supported as https://github.com/yoshuawuyts/bankai/blob/master/.travis.yml#L2 implies?

I think 4.0+ should be OK ✨ - basically targeting the latest LTS release


Is the current API untouchable? Changing to (opts) => {css, html, js} would make things considerably easier and would allow to get rid of most of the state stuff. Remains could be put into a state container

Great idea, let's change this. Perhaps a separate issue / PR?


Rehydrating choo is a bit clumsy, using replaceNode obviously is sub optimal. It would help a big deal if choo retained the id it hydrates on to support consecutive app.start(selector) calls

Could you elaborate? Why is replacing the script tag sub optimal?


Awesome work so far hey; super excited about this 😁

@marionebl
Copy link
Contributor Author

marionebl commented Aug 2, 2016

Hey @yoshuawuyts, thanks for your feedback. Glad you like the concept! If understand you correctly the way forward should be

Things to do to land this PR

  • Use the meow command line interface as is
  • Put some work into browserify.debug based on env, expose it as explicit option
  • Allow configuration of the HMR stuff as explicit option
  • Allow configuration of the isomorphic rendering / rehydration stuff as explicit option
  • Upgrade to node 4 capabilities (template literals, yeay! 🎉 )

Things to decide (and then do) to land this PR

  • Should we abstract the HMR stuff before landing this or test it and move it to a separate package then? Is the simplified HMR stuff good enough anyway or should we look into using browserify-hmr. I'd vote for using the simplified version, expanding on it and then moving it out later.
  • What assumption would you like to make regarding the entry point? I'd vote for the current signature and falling back to a generic mode if this fails. But then I'm a choo novice and really think you should decide on this as this will be ecosystem-shaping (assuming I understood the whole thing correctly and bankai should become the goto dev tool for choo)

Things to do next (in a separate PR)

  • Draft a nice recipe for API usage piping to disk
  • Do the bankai build command and adaptions needed for this to work

Things let over for later (in separate PRs)

  • Revisit the API with the goal the move stuff away from the state object
  • My suggestion: Move SSE-based HMR to own module

This could play out as pretty kick-ass development environment for choo apps – Looking forward to this! 🚀


Rehydrating choo is a bit clumsy, using replaceNode obviously is sub optimal. It would help a big deal if choo retained the id it hydrates on to support consecutive app.start(selector) calls

Could you elaborate? Why is replacing the script tag sub optimal?

What I meant when writing this was

var tree = app.start()
document.body.replaceChild(tree, document.body.firstChild)

See feat/cli/client-hmr.js#L45

This runs for each and every hmr update and loses all state of the current application that is not managed by choo models (e.g. focus). I thought it would be nice if app.start('selector') was idempotent, enabling me to do the following as often as I like after initial start:

var tree = app.start('[some-selector-here]')
if (tree) {
  document.body.appendChild(tree)
}

Alternatively this API would be nice too (given a guarantee the passed in element remains attached to the DOM)

var el = document.querySelector('foo');
app.start(el)

@marionebl
Copy link
Contributor Author

Hey @yoshuawuyts it would be dope if you found the time to give partial feedback; I've got some time this evening and could work on this then.

@yoshuawuyts
Copy link
Member

Almost done with feedback - today is a long day, haha. Posting in a sec

@yoshuawuyts
Copy link
Member

  • Use the meow command line interface as is
  • Put some work into browserify.debug based on env, expose it as explicit option
  • Allow configuration of the HMR stuff as explicit option
  • Allow configuration of the isomorphic rendering / rehydration stuff as explicit option
  • Upgrade to node 4 capabilities (template literals, yeay! 🎉 )

Woooh! 🎉


Should we abstract the HMR stuff before landing this or test it and move it
to a separate package then? Is the simplified HMR stuff good enough anyway or
should we look into using browserify-hmr. I'd vote for using the simplified
version, expanding on it and then moving it out later.

Aye, sounds good. The simplified HMR stuff is definitely simple enough - I
believe the redux people are even moving away from their complicated solution
because it's flaky. Also really into the idea of making it a standalone package
eventually.


What assumption would you like to make regarding the entry point? I'd vote
for the current signature and falling back to a generic mode if this fails.
But then I'm a choo novice and really think you should decide on this as this
will be ecosystem-shaping (assuming I understood the whole thing correctly
and bankai should become the goto dev tool for choo)

For choo I'd like the signature to be:

const html = require('choo/html')
const hmr = require('choo-hmr')
const choo = require('choo')

const app = choo()
app.use(hmr())

app.router([ '/', () => html`<div>hello world</div>`])

const tree = app.start()
document.body.appendChild(tree)

This is not possible with the current API though, so using a placeholder that
forces this behavior to bridge the next few weeks is a path we could take. The
other option is to fix the tests for
yoshuawuyts/barracks#59 so we can merge it and create a
magic-less plugin implementation. Which one would you prefer? We could also
hold off HMR for the first patch, and merge it in a couple of days once we get
the wrappers into barracks and choo. Does that make sense?


This could play out as pretty kick-ass development environment for choo apps
– Looking forward to this! 🚀

🎉


Rehydrating choo is a bit clumsy, using replaceNode obviously is sub optimal.

Have you seen choojs/choo#203? The API for
rehydrating becomes:

const mount = require('choo/mount')
const choo = require('choo')
const app = choo()

if (module.parent) {
  module.exports = app
} else {
  const tree = app.start()
  mount('[some-selector-here]', tree)
}

This runs for each and every hmr update and loses all state of the current
application that is not managed by choo models (e.g. focus)

Hmm, shouldn't morphdom be taking care of this? It feels there might be a
slight mismatch between how things work. Are you using the current id
seclector API at all, perhaps
https://github.com/yoshuawuyts/choo-handbook/blob/master/rendering-in-node.md
might be useful as a reference for the current API.


Sorry for taking so long to reply haha, hope this was helpful :D

@marionebl
Copy link
Contributor Author

marionebl commented Aug 4, 2016

Thank you, this clears things up quite a bit!

  • Let's postpone HMR until choo and barracks change settle in and go with the use api. That is obviously is the superior approach and warrants some delay. This also gives me some time to look into the idea of amok
  • Haven't seen app.start: clean choo#203, new API looks 👌
  • Thanks for the additional pointers regarding rehydration, I'll have a look at this

@yoshuawuyts
Copy link
Member

@marionebl just holler when you think this is ready to merge, and I'll be happy to 😁

@marionebl marionebl mentioned this pull request Aug 9, 2016
@marionebl
Copy link
Contributor Author

@yoshuawuyts moved the actual and feasible PR to #52, closing this and keeping the change history around for future reference

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.

2 participants