Require function is leaked to the global namespace since v2.0.3

[PaulTondeur]

As of version v2.0.3 the require function is leaked to the global namespace.

When I look at the current compiled js-yaml.js, it has the following code on line 24:

require=(function(e,t,n){function i(n,s) ......

It is missing the var definition and thus leaking the variable to the window. It should instead be:

var require=(function(e,t,n){function i(n,s) ......

This is especially a problem when using together with requirejs, as it breaks many things. When changing this locally, it works as expected (with or without requirejs)

I've tried forking the project so I could send you a ready to use fix, but I'm not able to find where this went wrong between version 2.0.2 and 2.0.3. I suspect this is something that goes wrong in the build process.
Fixing it might be achieved by putting the var in 40_before.js, but I think there will be some other place this should be fixed.

[ixti]

Sorry, I got out of the loop. @puzrin said we're not using browserify anymore. And forced me to delete my irrelevant comments :P

[puzrin]

@PaulTondeur , we don't support browser officially, that build is only for demo page and can contain some crap, that doesn't make sense for demo. As i see, browserification code is completely outdated.

However, code is written to be browserifiable. Try to use webmake as here https://github.com/nodeca/babelfish/blob/master/Makefile#L91. If everything will be ok, i'll fix makefile and rebuild code.

[PaulTondeur]

Thanks for the quick replies @ixti and @puzrin. Too bad browserify is not used anymore. Till yesterday I never heard of browserify or webmake. Node is also still new to me.
I came across this issue while helping a friend out streamlining the use of requirejs. As much as I'd love to dig deeper into webmake and learn about how you've set up and build this project, I won't be able to do so right now. I just made an attempt and failed at it. I'm already quite happy that I was able to pinpoint it to where it exactly did go wrong.
I'll point this friend to this discussion, maybe he can look into it for his project :-)

[puzrin]

I don't know, who really needs to use yaml in browser. But if someone wish to care about browserified version & AMD - no problem, good patches will be accepted. Personally, i'd recommend to use webmake, it produces more clean code.

Can we close this issue?

[ixti]

@PaulTondeur as a temporary measure you can use the patch I was proposing: 134de73

[PaulTondeur]

I think the beauty of making a good and solid library is that it can be used in ways you never imagined. In my case this is because the project is a pilot project for porting a large flash project seamlessly over to HTML5. Sure a tool could be used to save the yaml files as a different format, but I also understand that it would be easy to use the exact same data-source. There might be many other reasons why one would like to use yaml on the frontend. So I would vote for caring about a browserified version.

In this case js-yaml is installed by using bower. Sure I can apply the fix I proposed and was implemented by you @ixti, but it would be nice if it was correct at the source, as this completely breaks the bower setup. There will be ways around it, but its not that elegant to do so (read: make an exception for js-yaml and put a manually edited version into the GIT repo of the project and not use bower for it).

To be honest I think it is too soon to close this issue. It is still an issue with js-yaml that it leaks the require method (in the browser ready version). In this thread we have discussed how it could be resolved. I would say that an issue could be closed if:

• The issue is resolved
• The feature request is denied
• The question is answered

Neither is the case at this moment.

[ixti]

I tend to agree with you @PaulTondeur.
@puzrin I played with webmake a little bit and it works just fine with only few bits needs to be done:

• move require patches out of main lib file (./lib/js-yaml.js) to probably index.js at least it works and looks better in this case.
• Fix binary type to play nice with browsers (due to Buffer class).

Will try to provide a PR.

[PaulTondeur]

Great to hear we tend to agree @ixti :-)

A small addition to my previous comment is that this other Flash project we're talking about made use of yaml as its main datasource. I did not expressed that very clearly in the comment before.

I'd really like to help out and do not intend for someone else to do it. But node and all of its tools is too new for me to be effective at it at the moment. It is highly appreciated that you've had a look into this already and see ways to get this addressed!

[puzrin]

Should be fixed in refactoring branch, now supports AMD and fallbacks to existing AMD systems.