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

Made paths more consistent between client and server and friendly to RequireJS. #173

Merged
merged 2 commits into from
Nov 7, 2013

Conversation

alexindigo
Copy link
Member

Spike,

This is next iteration of making Rendr play nice with RequireJS.

I made grunt script to fetch-n-wrap all the modules from rendr folder needed in the browser,
despite verbose config (and supposedly fixed bug) it works fine.

So it's first subset of changes to make Rendr truly client-side citizen.

Let me know what you think and if you ok with this direction.

Thank you.

/cc @jskulski
/cc #155

@spikebrehm
Copy link
Member

Thanks for all your work on this, @alexindigo. I'll be able to take a look tomorrow.

It strikes me that we ought to have automated browser testing to assert that the various module systems still work with each change. I made some progress towards a Sauce Labs + Travis CI setup a few months ago, but this needs to be revisited.

@alexindigo
Copy link
Member Author

Yep, it's good idea. Do you have your frontend tests in this repo?

@c089
Copy link
Member

c089 commented Nov 6, 2013

I might be able to help on that effort. We actually have the unit tests for our rendr app running in node and various browsers using mocha, karma and browserstack.

@@ -6,5 +6,6 @@ if (typeof window !== "undefined" && window !== null) {
};
} else {
global.isServer = true;
global.rendr = require('../index');
var serverOnly_rendrIndex = '../index';
Copy link
Member

Choose a reason for hiding this comment

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

In each of these cases, can you add a comment just to clarify that this is done to support the r.js optimizer?

@spikebrehm
Copy link
Member

Thanks @alexindigo! This looks good. My only requested change is to add comments to make it more clear when the hack to support Require.js is necessary, to make it easier to maintain down the line with more contributors.

Also, it would be really nice to see a little sample app that is using Require.js, so I can test it out (otherwise I'm flying blind). It could even be great to add to the examples/ dir. 05_requirejs!

@alexindigo
Copy link
Member Author

Thanx. I added comments.

Yes, example site is great idea, will definitely do.
But this changes is the first step, to make it compile and load without errors,
next step will be to make it more async (at the moment I still need to preload all the controllers I have in my routes).

When it's done, I'll add new example – it will help us test things without trulia specific hacks.

spikebrehm pushed a commit that referenced this pull request Nov 7, 2013
Made paths more consistent between client and server and friendly to RequireJS.
@spikebrehm spikebrehm merged commit 69e408f into rendrjs:master Nov 7, 2013
@alexindigo
Copy link
Member Author

@c089 do you mind to share your test suite? I have more changes in the pipeline, it would be nice to use your thing to make sure it's still compatible with stitch-powered implementation. Thanx.

@c089
Copy link
Member

c089 commented Nov 7, 2013

Well our test suite tests our application so that wouldn't have much value ;) But I'll try to get the rendr tests running in browsers and also provide example unit tests for the example app soon

@alexindigo
Copy link
Member Author

Cool. Thanx a lot.

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.

3 participants