oc: RequireJS and Oc-client don't really go together

This is an interesting problem we ran into while integrating components on a different stack which uses requirejs. There are two different kind of errors we see -

When clientJS is script is included after the requireJS (which is bundled with other stuff) we see the following.

Uncaught Error: Mismatched anonymous define() module: function (exports, $) {
      factory((root.oc = exports), $, root.head, root.document, root.window);
    }

Referring to - https://github.com/opentable/oc/blob/master/src/components/oc-client/_package/src/oc-client.js#L10

When ClientJS is included before RequireJS but the tag is included later or using js to render a component, we see the following problem -

Error rendering component: http://registryurl/tutor-list/1.0.4?subject_id=43&limit=5&actions.new_tab=1&__oc_Retry=0, error: TypeError: Cannot read property 'template' of undefined

Basically referring to https://github.com/opentable/oc/blob/master/src/components/oc-client/_package/src/oc-client.js#L350

Thoughts? The 2nd problem seems like something we should be able to fix with the way we use handlebars when define is present ?

About this issue

Original URL
State: closed
Created 7 years ago
Comments: 17 (11 by maintainers)

Most upvoted comments

It looks like we may have a ~~good~~ solution:

define(['exports', 'jquery'], function(exports, $) {
  root.oc = $.extend(exports, root.oc);
  factory(root.oc, $, root.head, root.document, root.window);
});

what do you think @navamgupta and @matteofigus ?

mattiaerre on May 10, 2017

@matteofigus I am going to create a PR for this as I think it is a good starting point. thank you very much @navamgupta for your help; this has been really a joint effort 🙏

No No, no problem at all and thanks a lot for spending a good amount of time on this with me 😃 Not to mention a great crash course on requireJS and AMD.

navamgupta on May 10, 2017

@navamgupta what about this as a possible solution?

https://github.com/mattiaerre/oc-hub/pull/10/files#diff-6e8c5c408e77c3d6b1af80d121354c87R11

the line you mentioned enables requirejs to have an instance of the ocClient available as a parameter of the main callback (so you can use that instead of the global oc object)

mattiaerre on May 10, 2017

@mattiaerre The code that does not work is not really a mix and match of different styles. Its just a combination of -

Having a component which uses oc.cmd.push to wrap its js code.
Server side rendering this component
Loading the client.js (using proper require ways as you have explained above) after the above ssr markup.

The script tag that I asked you to try out could be the SSR markup coming from a component right? Maybe I am completely missing some point here, so I guess I should ask what according to you is the mix and match of styles happening here? If it is the fact that code coming from the component (the script i asked you to insert) itself is not using require then my argument would be that it does not make sense to re-write a component to use require just because one of the consumers (3rd party) uses require. Frankly I think consumer side js frameworks should not impact individual component code. The way you include the clientJS may change but the component’s code should not.

On a side note, while we discuss the above I also want to understand why we have root.oc = exports in this line - https://github.com/opentable/oc/blob/master/src/components/oc-client/src/oc-client.js#L11 Is it an AMD/RequireJS standard ? If yes what does it really do?

navamgupta on May 10, 2017

I’m trying to reproduce the issue described above, with the following PR:

https://github.com/mattiaerre/oc-hub/pull/10

I’ve got 2 components w/ SSR and a component that is rendered client side w/ require.js at the end of the html body.

narrowing down the cases it looks like it might be a problem w/ oc.cmd.push (@navamgupta to confirm)

// cc @matteofigus

mattiaerre on May 9, 2017

for 1 will wait on what others have to say. for 2 I think the problem is

if(typeof exports === 'object' && typeof module === 'object')
		module.exports = factory();
	else if(typeof define === 'function' && define.amd) 
		define([], factory);    // ends up going here
	else if(typeof exports === 'object')
		exports["Handlebars"] = factory();
	else
		root["Handlebars"] = factory(); // instead of here

Snippet is from the non minified version of the handlebars we load for oc - https://cdnjs.cloudflare.com/ajax/libs/handlebars.js/4.0.6/handlebars.runtime.js I think handlebars wants to support amd all the way, in a sense that if amd is defined handlebars will not be exposed globally and our problem is we expect it to be available globally

navamgupta on Apr 25, 2017