-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Requirejs server support #868
Conversation
There are two issues with the current 'amd' support. Firstly, the module is being defined with a moduleId. It should be defined anonymously to allow for module name mapping within an amd loader. I have made this change as well. Secondly, the amd definition should fire after the module has been constructed, not before. With the current setup, any modules using this as a dependency will get an empty variable that hasn't been populated yet. To properly call 'define' only after the entire module has been constructed would involve reconstructing the entire source slightly. A sample best practice form is provided here - https://github.com/volojs/volo/wiki/Library-best-practices#wiki-single I've made the above adjustment for my own forked version of the code and will be using this until comprehensive amd support is provided by less.js. The primary reason I use less.js over everything else is for this cross-platform support which no other css compilers seem to offer. Getting on board properly with amd is important to properly leverage this advantage. |
@@ -35,7 +35,7 @@ less: | |||
${SRC}/tree/*.js\ | |||
${SRC}/tree.js\ | |||
${SRC}/browser.js >> ${DIST} | |||
@@echo "})(window);" >> ${DIST} | |||
@@echo "})(typeof window === 'undefined' ? undefined : window);" >> ${DIST} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this trying to achieve? if the typeof window is undefined then use undefined, otherwise keep the same... I struggle to see any difference introduced by this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the server, 'window' does not exist. So attempting to use the variable straight will throw an error. 'typeof' allows us to check if it exists without throwing an error.
Thanks for taking the time to look into this. It is a very valuable asset to less to get its amd support right. In many ways, the pull request above is a suggestion, and this process needs to be considered a bit more carefully depending on the specific module needs and constraints of this file, of which I am not entirely aware. Many of the changes are quick fixes as opposed to a thorough consideration of the best way to package the module. The best way to support amd would be to alter the structure of the factory function a bit more drastically. The best example of how to write an AMD module that works in node, the browser and server-side requires is the following (this is an official RequireJS repo): https://github.com/umdjs/umd/blob/master/returnExports.js This template would be the ideal case. |
moving discussion to #933 as this pull request cannot be accepted in its current form. |
I'm a big fan of having codebases that can work equally well on client and server, and less.js is one of them.
In a requireJS project, there is no reason I shouldn't be able to use exactly the same version of less.js both in the browser and on the server.
The elegance of the following is what I am after:
And then having:
work both on the client and server, with the same codebase.
I've got this working on my own internal code, and have included the minor corrections needed in this pull request.
I completely understand if this doesn't make sense to your own process, but I'm about to release the client and server-side compatible requireJS plugin, require-less, and this would mean the difference between being able to have less as a dependency and needing to include all the code with this fork being kept up to date.
Would be much appreciated if you can take any of this on board.