Skip to content

Subclass HTTPService template#52

Merged
jnthn merged 21 commits intocroservices:masterfrom
japhb:subclass-template
Jan 9, 2018
Merged

Subclass HTTPService template#52
jnthn merged 21 commits intocroservices:masterfrom
japhb:subclass-template

Conversation

@japhb
Copy link
Contributor

@japhb japhb commented Dec 11, 2017

This adds the ReactReduxSPA template, subclassing HTTPService, based on the non-app-specific parts of croservices/sample-app-spa-react . It also includes a minor refactoring of new directory creation (mostly in Common) to make this cleaner.

It does not include creation of .gitignore files as I have a tickle at the back of my brain that my initial design idea wasn't quite right, so I'd like to just get this piece merged while I ruminate on that.


CODE
$contents ~= q:to/CODE/ if %options<websocket>;
[].forEach(endpoint => {
Copy link
Member

Choose a reason for hiding this comment

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

This loop over an empty array is a no-op; forget to include something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two answers:

  1. Nope, it's (intentionally) a stub to be filled in by the user when they have defined websocket endpoints.
  2. This could be filled in better even in the stub form by explicitly hooking up the "chat" websocket code on both ends, which I have not (yet) done.

Do you want me to do #2 before merging the PR, or is #1 satisfying to you already?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer #2 if it's OK with you, though if you'd much prefer to put it off until later, I can merge this as is.

Further improve subclassibility of the HTTPService template by splitting out
the static and websocket route contents from the app-module-contents method.
Serve the cro-generated `static/index.html` file and the webpack-generated
`static/js/bundle.js` bundle file instead of the inherited raw HTML snippet.

(Actually, the route code staticly serves everything below `static/js/`, in
case the user wants to serve some common libraries separately, but in this
case the stub project will only have the webpack bundle.)
This quiets a warning in the Firefox web developer console and follows the
w3.org recommendation.
@japhb
Copy link
Contributor Author

japhb commented Dec 22, 2017

I made several commits last weekend, doing all the prep work just shy of actually accomplishing #2. Unfortunately, I'm about to be AFK (at least, away from my Perl 6 hacking keyboard) for an extended period -- so I think I'd like to merge what's already done on this PR if possible, and restart on #2 with a fresh PR branch from master after I'm back to the hack.

@jnthn
Copy link
Member

jnthn commented Dec 23, 2017

@japhb Just been out of things for over a week myself with flu...will look at this again in the next days and, unless I spot anything really troublesome, will merge.

@japhb
Copy link
Contributor Author

japhb commented Jan 9, 2018

Ping? I'm not back to full-on Cro hacking yet (so my merge request still stands) but I am back near a laptop again, so I can answer any questions you might have had.

@jnthn
Copy link
Member

jnthn commented Jan 9, 2018

Sorry, got forgotten during the flu/holiday season. Just looked over it again now, and I don't see any reason not to merge. So...

@jnthn jnthn merged commit 8578342 into croservices:master Jan 9, 2018
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