Flotype / now

NowJS makes it easy to build real-time web apps using JavaScript
http://www.nowjs.com
MIT License
1.91k stars 175 forks source link

please don't search and replace against the lib/client/now.js file [from kodingen] #132

Closed humanchimp closed 13 years ago

humanchimp commented 13 years ago

.... it causes truly horrendous headaches....

this fix should be sufficient... if you have any issues with this suggestion, please let me know... i am happy to help...

thanks, chris

steveWang commented 13 years ago

Why not just statically host the file after it's been populated with server-specific info?

humanchimp commented 13 years ago

would you mind explaining what the motivation of the search and replace is to begin with, and why it cannot be solved with normal JS variables? to me, the search and replace bit A) is not necessary B) is a pain-point for deployment and C) is not a good practice in general. Can you give me a clear understanding of why it's being done in the first place? I would love to hear it :) To me, having an invalid .js file in the baseline, that is likely to change and require manual updating between versions, is a totally unreasonable thing to ask of your users. The fact is that is best to serve files that aren't likely to already be stored in someone's browser cache as a single, minified source in a production environment. making that a manual step, as you are suggesting, especially when managing separate environments (development, local, production, etc.), is wrong, and a bug. If you can explain how my patch does not resolve this once and for all, then I am more than happy to invest my own time in improvements and bug fixes, as necessary.

steveWang commented 13 years ago

Well, for one, window.location is set on a per-window basis, so if you wanted NowJS to run on a different server (port, even) than whatever's serving the HTML file, then your particular "fix" just wouldn't work, and you'd have to host the file statically.

The **SCOPE** part defaults not to this, but to window, first of all, and second of all, it allows the end-developer to specify in which scope he wants the now namespace -- essentially, it allows for multiple now namespaces on a given page, provided that the individual servers have this option configured properly.

**OPTIONS** permit the end-developer to set the default client-side options. At the moment this consists only of things to pass in to Socket.IO, but this may change in the future.

And either way, it's not like the search-and-replace happens every time a user connects; we cache the file server-side, so as long as the server itself is running, it only does this replacement once.

humanchimp commented 13 years ago

First of all this === window. if you don't believe me, why don't try it for yourself, instead of assuming that I just don't know what I am talking about? I changed the last line of your script to call the outer function in the context of the outer scope (this, which === window in a script that was included by a <script> tag). If you would rather refer explicitly to "window" then I invite you to do that. In the meantime, open your browser's JS console and type the following:

this === window true

When I spoke about this with you in IRC, you said that **SERVER** and **PORT** were only the defaults, which could be overridden. If that's not the case, then maybe my exact suggestion, or "fix" as you would prefer to phrase it, is not ideal.

That said, there are surely other ways of initializing a script than search-and-replace against a source file and hardcoding values! One of the simplest ways to do it is parameter passing. Another way to do it is by setting properties on an object. Either way, if this wasn't a pain point for us, I surely wouldn't have spent this much time on it.

If you guys update on your end, we need to go through and regenerate the static file (by hand, since there seems to be no good way to do it—please correct me if I am wrong.) This is true especially when you do a ground-up rewrite like 0.6 to 0.7, but it is true even when there is only a trivial change made, so as to assure that the client side code doesn't fall out of sync with what the server side is "expecting". Since it's not at all necessary, that extra step "sucks".

You could do something like this, and avoid wanton search-and-replace against the source code:

<script src="/path/to/now.js"></script>
<script>
now.someAmazingInitMethod({
  hostname: 'example.com'
, port: 80
, scope: foo
, options:  {
     bar: 10
   , baz: 'kule!'  
  }
});
</script>

Finally, you bring up an interesting point. You say that the purpose of search and replace here is to be able to include "multiple now namespaces" on the same page. OK, but it's still not necessary to include all the source code again and again. Forgive me if I am laughing about this. Is this really the approach you are recommending?

<script src="now.some-config.js"></script>
<script src="now.some-other-config.js"></script>
<script src="now.some-other-other-config.js"></script>
<script src="now.some-other-config-even-still.js"></script>

If so, then thanks for the funny. Even if you concatenated all those files together, it is a violation of a well-known computer science principle called "Don't Repeat Yourself".

But jokes aside, please be open-minded enough to consider that there is a better solution than search-and-replace.

ericz commented 13 years ago

Also this commit: https://github.com/Flotype/now/commit/fa618a379288bc392347fdb77f404dd1d9dd88b5

ericz commented 13 years ago

@humanchimp:

You can now use /lib/client/now.js directly from Github. Search & replace is no longer done so you do not need to modify the client library for use.

By default the client library (as it is on Github) will not try to make any connections. You have to call window.nowInitialize in order to initialize now. nowInitialize will return the now object. Parameters are the URI to connect to and client side options. If you leave out both parameters, the URI defaults to the current host and port (of the html page)

Use example:

 <script src="mycdn/now.js"></script>
 <script>
   var now = nowInitialize('http://mysite.com:9000', {});  // Both parameters are optional. URI defaults to '/' and the options parameter defaults to '{}'
 </script>

autoHost through the now server is still supported as before. Instead of search and replace we append a (scope).now = nowInitialize( (host):(port), (options) ); to the bottom of the /nowjs/now.js file.

I hope this addresses all of your concerns. Let me know if you have any further questions