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

remove name from define calls #356

Closed
wants to merge 2 commits into from
Closed

Conversation

Stereobit
Copy link
Contributor

Hey,

with this commit there was a name added to the define calls for require js: afc7e15

This is a bad idea because the name is usually added by the optimization tool. With a fixed name you run into problems in a lot environments when requiring the files. See http://requirejs.org/docs/api.html#modulename for more informations.

Also the name for hammer.js was only added to the dist file but not to the outro.js. This is way I touched the dist file.

Cheers
Tobi

@jtangelder
Copy link
Member

this is changing too often (with name, without name, some other tricks...), i need to read in about this. I havent got any experience with this for clientside libraries, can you point me a good place to start?

@Stereobit
Copy link
Contributor Author

Hey, Sitepoint offers a good article to start with this topic: http://www.sitepoint.com/understanding-requirejs-for-effective-javascript-module-loading/ And the official documentation under http://requirejs.org/ for sure.

If I can help you with some explanations on this topic don't hesitate to contact me :-).

Maybe @jantimon who did the commit I mentioned above can check my changes too?

Cheers
Tobi

@jantimon
Copy link

Unfortunately this won't work if you use the jquery.hammer.js file which contains both define calls in the same file: https://github.com/EightMedia/hammer.js/blob/master/dist/jquery.hammer.js

This might not a big issue as you could still load the files separately however you should keep in mind that you would have to write a specific requirejs configuration or move hammer.js and hammer.jquery.js into the same directory.

@Stereobit
Copy link
Contributor Author

Yes, that's true. But having two define calls in one file is wrong anyway. If you want to provide a version with the library and the jquery plugin in one file you should add one define call at the end of it. When using a module loader merging files and adding module names should only be done as part of the build process.

@jantimon
Copy link

My intention was to fix the #331 issue without changing the existing code as Hammer.js is also used for non AMD projects.

In my projects I am using the r.js optimizer it works very well with the applied patch:

http://jsfiddle.net/krismeister/A2fuu/3/

@jantimon
Copy link

In the latest Hammer.js version the name is removed and therefore requirejs doesn't work anymore:
http://jsfiddle.net/A2fuu/4/

@Stereobit
Copy link
Contributor Author

Yes. And no, hehehe. Your definitely right. Without the name the first define is the only define called which is a problem. But we should not fix it with adding the name but rather with having only one define for the merged require-jquery file. I'll try to change the build config to a working solution. Not sure if it's possible.

As long as it's not - the solution for your problem is just using requirer in it's normal way. Load hammer and jquery.hammer separately. Every module is in one file which is not a problem since r.js will merge it anyways. Check it out: http://jsfiddle.net/uAqXY/

Hope that helps :-)

~ Tobi

@jantimon
Copy link

That's true, actually that is also the way I am using right now.
What I don't like is that it won't work out of the box - you have to look up how to setup the config for requirejs

But it requires two settings in the requirejs configuration.

With a smart folder structure you would have to define only the location of hammer.js and could also require hammer/plugins/hammer-jquery

@Stereobit
Copy link
Contributor Author

See » #364

@jtangelder
Copy link
Member

merged #364, this became obsolete

@jtangelder jtangelder closed this Sep 19, 2013
@jtangelder
Copy link
Member

Issuebot:
This issue is auto-closed because it's last activity was too long ago.
If you think the issue is still active, please comment to re-open it!

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.

None yet

3 participants