Made Rendr work with requirejs #190

alexindigo · 2013-11-19T22:14:34Z

@spikebrehm This PR adds RequireJs support to Rendr and example app 05_requirejs that contains grunt file and modified app files.
In order to make it run, you need to augment Rendr files to work on frontend by running grunt init (as you suggested no extra boilerplate in original Rendr files on the server).
When it's done you can continue as usual with grunt server.
This example works with unminified/uncombined app/rendr files as a proof of the concept (and probably dev env workflow), for the production files can be minified combined into one or few different files using grunt + r.js minification tool.

Please let me know if you have any questions/concerns. Thank you.

/cc @bigethan @jskulski

spikebrehm · 2013-11-19T23:17:36Z

.gitignore

@@ -7,5 +7,9 @@ examples/*/node_modules
 examples/**/compiledTemplates.js
 examples/*/public/mergedAssets.js
 examples/*/public/styles.css
+examples/*/public/mergedAssets.js


This line is duped from above

spikebrehm · 2013-11-19T23:25:50Z

examples/05_requirejs/public/js/app.js

@@ -0,0 +1,20 @@
+require(


should this file be removed?

we need to preload globals before everything and routes too,
handlebars require in the compliledTemplates.

spikebrehm · 2013-11-19T23:34:49Z

In general I really like the approach. It's good to make more of these methods async (BaseView, Fetcher). Like I said, I don't have the chance to spin through it right now locally because of a deadline but LMK if I'm blocking you on anything.

One note: it would be awesome if these changes could conform to the existing style of the codebase. We try to follow the style guide published here: https://github.com/airbnb/javascript. Mainly i'm thinking of having curly braces on the same line rather than on its own line (PHP-style).

Good:

if (foo) {
  bar();
} else {
  baz(function() {
    console.log('baz');
  });
}

Bad:

if (foo)
{
  bar();
}
else
{
  baz(function()
  {
    console.log('baz');
  });
}

alexindigo · 2013-11-19T23:39:56Z

Sure, I fix styling. It's just helps me to read code, usually I go and clean up after, but when it's middle of the night, I might miss some chunks. :)

For I'm focusing on other stuff too,
but it would be nice to take it out of limbo one way or another
and sooner the better.

Let me know what I can do to speed things up.

Thank you.

keeping it by default, but adding ways to disable it. Useful for Oauth, which doesn't like JSON requests. PS. We need it now, but I'll clean it up and make as separate PR, when we will be merging RequireJS one.

spikebrehm · 2013-11-27T19:23:47Z

client/router.js

+  {
+    // TODO: Make it function (err, View)
+    if (!_.isFunction(View)) {
+      throw new Error("View '" + key + "' not found.");


Probably should pass this error as first arg to callback rather than throwing.

spikebrehm · 2013-11-27T20:31:16Z

I'm having some trouble running 05_requirejs example locally. Getting this error in js console:

GET http://localhost:5000/js/libs.js 404 (Not Found)

and indeed, public/js doesn't have it:

$ ls public/js/
total 8
drwxr-xr-x  3 spike  staff   102B Nov 27 12:26 app
-rw-r--r--  1 spike  staff   336B Nov 27 12:01 app.js

spikebrehm · 2013-12-10T18:58:36Z

Cool. Looks good. There's some cleanup we can do, but let's just get this merged and move forward from there! Thanks a lot @alexindigo @jskulski @bigethan for all your help.

Would you like to try squashing these commits first? If not, not a blocker.

alexindigo · 2013-12-11T00:36:43Z

Sure, thank you for your support.

Made Rendr work with requirejs

alexindigo · 2013-12-11T00:52:49Z

Oh... I just squished everything together :) Too slow. #fail :)

spikebrehm · 2013-12-11T01:00:14Z

Lol, sorry! I was too eager!

alexindigo · 2013-12-11T01:01:53Z

It's actually good. Keeps me on my toes :) You know – survival of the fittest :)
Thanx one more time. :) It was awesome ride. :)

spikebrehm · 2013-12-11T01:16:46Z

Ah shoot, ran into an error with grunt init. Should have tried earlier:

Running "rendr_requirejs:init_shared" (rendr_requirejs) task

Tracing dependencies for: rendr/shared/app
Error: ENOENT, no such file or directory '/Users/spike/code/rendr/examples/05_requirejs/public/js/rendr/shared/rendr/shared/rendr/client/app_view.js'
In module tree:
    rendr/shared/app

{ [Error: Error: ENOENT, no such file or directory '/Users/spike/code/rendr/examples/05_requirejs/public/js/rendr/shared/rendr/shared/rendr/client/app_view.js'
In module tree:
    rendr/shared/app

    at Object.fs.openSync (fs.js:427:18)
]
  originalError:
   { [Error: ENOENT, no such file or directory '/Users/spike/code/rendr/examples/05_requirejs/public/js/rendr/shared/rendr/shared/rendr/client/app_view.js']
     errno: 34,
     code: 'ENOENT',
     path: '/Users/spike/code/rendr/examples/05_requirejs/public/js/rendr/shared/rendr/shared/rendr/client/app_view.js',
     syscall: 'open',
     fileName: '/Users/spike/code/rendr/examples/05_requirejs/public/js/rendr/shared/rendr/shared/rendr/client/app_view.js',
     moduleTree: [ 'rendr/shared/app' ] } }

spikebrehm · 2013-12-11T01:17:09Z

The path is weird for the command: /public/js/rendr/shared/rendr/shared/rendr/

alexindigo · 2013-12-11T01:21:28Z

yes, it's a bug in requires (they said it's not, but rather feature) I'm yet to add workaround for that, I mean I have workaround there as part of grunt options, but going to add into requirejs itself.

Can you show me your grunt options file or is it the one from 05_requirejs?

spikebrehm · 2013-12-11T01:22:41Z

It's straight from master.

alexindigo · 2013-12-11T01:23:26Z

Hmm, one sec then.

muhleder · 2013-12-11T01:25:56Z

I ran into this just today. You need to add
'rendr/client': '../../../client',
to the paths for init_shared in the Gruntfile.

muhleder · 2013-12-11T01:27:41Z

It comes from getAppViewClass() referencing the client libs in rendr/shared/app.js

alexindigo · 2013-12-11T01:31:17Z

I think I added it. And I had to remove and recheckout my example/05 to reproduce it (lack of smoke tests).
Thanx :)

alexindigo · 2013-12-11T01:33:33Z

Oh and tests, seems like I forgot something somewhere. Sorry.

spikebrehm · 2013-12-11T01:35:25Z

Thanks a lot @muhleder! That does it. Fixed in 34d2563.

There may be some other things we need to fix due to divergence from master during this PR's long incubation period. For example, I see a reference to rendr.entryPath in users_controller.js:

https://github.com/airbnb/rendr/blob/master/examples/05_requirejs/app/controllers/users_controller.js#L55

We've since deprecated rendr.entryPath. I also wonder what workarounds are still left here that we can clean up.

spikebrehm · 2013-12-11T01:39:55Z

You know, actually, I think i might just remove the show_lazy action from all the examples, to slim them down, and I'll create a separate example to demonstrate lazy loading of models.

alexindigo · 2013-12-11T01:40:30Z

I'll port changes from example/00. (Feels like we're talking about Evangelion) :)

alexindigo · 2013-12-11T01:42:02Z

I think it's good to have more features to showcase in requires version, or maybe to have two examples, very simple one and something more sophisticated.

alexindigo · 2013-12-11T01:43:23Z

How about you'd update 00 – 04 the way you think would be more example-friendly and then I'll port `00`` and something else to requirejs.

spikebrehm · 2013-12-11T01:44:54Z

Deal, sounds good.

spikebrehm · 2013-12-11T04:29:01Z

@alexindigo any plans to release grunt-rendr-requirejs as an NPM module?

alexindigo · 2013-12-11T05:03:07Z

Yes, I wanted incorporate that workaround into it, but probably better "ship it early fix it later". :)
Will do.

alexindigo · 2013-12-12T08:26:32Z

Done. https://npmjs.org/package/grunt-rendr-requirejs
(Still work in progress though)

spikebrehm · 2013-12-12T16:31:28Z

Cool! It's a start. Added to 05_requirejs's package.json here: ec6632f

alexindigo · 2013-12-12T17:32:58Z

Thanx. Btw, there is PR for grunt-rendr-stitch, tests were failing. Or stitch is thing of the past now?

alexindigo · 2013-12-12T19:20:32Z

Oh and rendr-handlerbars, looks like it's missing recent changes from the npm package.

alexindigo added 7 commits November 7, 2013 17:14

Made controller lazy load.

41b4c70

Fixed typo.

129cd44

Dried stuff up.

f1827bb

Added comment.

2654324

Another workaround for requirejs after-thought-conversion.

53f58e1

Added rendr+requirejs example site.

660abf9

Updated Rendr to load and preload views, models and collections async.

c2ce265

alexindigo mentioned this pull request Nov 19, 2013

Made controller lazy load. #177

Closed

spikebrehm reviewed Nov 19, 2013
View reviewed changes

Cleanup.

f86cff3

spikebrehm reviewed Nov 19, 2013
View reviewed changes

Housecleaning.

f815242

alexindigo added 3 commits November 20, 2013 16:51

Cleanup. Always call callback in view.hydrate. Thanx @bigethan

d85fb61

FIxed Gruntfile after cleanup.

a567ece

For some request we don't need JSON,

9cd769a

keeping it by default, but adding ways to disable it. Useful for Oauth, which doesn't like JSON requests. PS. We need it now, but I'll clean it up and make as separate PR, when we will be merging RequireJS one.

spikebrehm reviewed Nov 27, 2013
View reviewed changes

spikebrehm mentioned this pull request Dec 10, 2013

Updating the examples to work with the new app_view override #209

Merged

spikebrehm pushed a commit that referenced this pull request Dec 11, 2013

Merge pull request #190 from alexindigo/master

ebe734c

Made Rendr work with requirejs

spikebrehm merged commit ebe734c into rendrjs:master Dec 11, 2013

alexindigo mentioned this pull request Dec 11, 2013

Missing cornerstone. #215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made Rendr work with requirejs #190

Made Rendr work with requirejs #190

alexindigo commented Nov 19, 2013

spikebrehm Nov 19, 2013

spikebrehm Nov 19, 2013

alexindigo Nov 19, 2013

spikebrehm commented Nov 19, 2013

alexindigo commented Nov 19, 2013

spikebrehm Nov 27, 2013

spikebrehm commented Nov 27, 2013

spikebrehm commented Dec 10, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

muhleder commented Dec 11, 2013

muhleder commented Dec 11, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 12, 2013

spikebrehm commented Dec 12, 2013

alexindigo commented Dec 12, 2013

alexindigo commented Dec 12, 2013

Made Rendr work with requirejs #190

Made Rendr work with requirejs #190

Conversation

alexindigo commented Nov 19, 2013

spikebrehm Nov 19, 2013

Choose a reason for hiding this comment

spikebrehm Nov 19, 2013

Choose a reason for hiding this comment

alexindigo Nov 19, 2013

Choose a reason for hiding this comment

spikebrehm commented Nov 19, 2013

alexindigo commented Nov 19, 2013

spikebrehm Nov 27, 2013

Choose a reason for hiding this comment

spikebrehm commented Nov 27, 2013

spikebrehm commented Dec 10, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

muhleder commented Dec 11, 2013

muhleder commented Dec 11, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

spikebrehm commented Dec 11, 2013

alexindigo commented Dec 11, 2013

alexindigo commented Dec 12, 2013

spikebrehm commented Dec 12, 2013

alexindigo commented Dec 12, 2013

alexindigo commented Dec 12, 2013