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

Support AMD in a transparent manner so that it can be used with requireJS #933

Closed
lukeapage opened this issue Sep 1, 2012 · 16 comments
Closed

Comments

@lukeapage
Copy link
Member

This is a big task and would involve large refactorings of less.

This issue is created to take over #868 which is not suitable to pull.

@guybedford
Copy link
Contributor

Great to see these directions being taken on board. Hearing that you're open to this kind of work, I'll see if I can manage a more complete attempt at this if I can find the time as well.

@lukeapage
Copy link
Member Author

Before you do something you should suggest a plan and what it will break if anything, then get us to approve. Otherwise you might be wasting your time.

@guybedford
Copy link
Contributor

Sure, makes sense. I can't promise anything either, as I'm quite overcommitted right now, but will see how things go.

@guybedford
Copy link
Contributor

Looking into the amd support again, the change above for flexible naming will provide perfect amd support in the browser.

This issue was previously generated by considering a combination of amd support in the browser and server at the same time - allowing the same library to be used in the same way. I like this generally with utility modules like this as one doesn't need to do things differently between the two environments.

But it's this providing amd support on the server that would need a more major refactoring. Perhaps rename this issue to reflect this. Once the above pull request is accepted, the browser amd support is fully compatible.

@guybedford
Copy link
Contributor

For the server situation, I have done the following in my fork:

  1. Included the adjustments as documented at Requirejs server support #868 turning the browser version of less-1.3.1.js into something that can run ont the server.
  2. Created a server file, less-server.js, which contains the following:
define(['./lessc'], function(less) {
    var fs = require.nodeRequire('fs');
    var path = require.nodeRequire('path');

    less.Parser.importer = function (file, paths, callback, env) {
        console.log('importing ' + file);
        var pathname, data;

        // TODO: Undo this at some point,
        // or use different approach.
        var paths = [].concat(paths);
        paths.push('.');

        for (var i = 0; i < paths.length; i++) {
            try {
                pathname = path.join(paths[i], file);
                fs.statSync(pathname);
                break;
            } catch (e) {
                pathname = null;
            }
        }

        paths = paths.slice(0, paths.length - 1);

        if (!pathname) {
            if (typeof(env.errback) === "function") {
                env.errback(file, paths, callback);
            } else {
                callback({ type: 'File', message: "'" + file + "' wasn't found.\n" });
            }
            return;
        }

        function parseFile(e, data) {
            if (e) return callback(e);
                env.contents = env.contents || {};
                env.contents[pathname] = data;      // Updating top importing parser content cache.
            new(less.Parser)({
                    paths: [path.dirname(pathname)].concat(paths),
                    filename: pathname,
                    contents: env.contents,
                    files: env.files,
                    syncImport: env.syncImport,
                    dumpLineNumbers: env.dumpLineNumbers
            }).parse(data, function (e, root) {
                    callback(e, root, pathname);
            });
        };

        if (env.syncImport) {
            try {
                data = fs.readFileSync(pathname, 'utf-8');
                parseFile(null, data);
            } catch (e) {
                parseFile(e);
            }
        } else {
            fs.readFile(pathname, 'utf-8', parseFile);
        }
    }
    return less;
});

This updates the importer property for server import support.

The above means that a single version of less can be used both browser and server side as an amd module, with a minor adjustment on the server as shown above.

With the above concept, one could create:

  • A multi-distribution version - working in Node and the browser seamlessly within one file. The size difference would only be the above code (~20 lines or so I think)
  • If the above seems too much, it is possible to create the entire Node AMD version in one file, which is much more convenient for server amd than trying to bootstrap through all the development files.

These are my suggestions, happy to assist where you want to take things, or leave as is as well.

@guybedford
Copy link
Contributor

Just to summarize the above point a bit better - I may have been a bit rushed in writing this.

The goal here is to create either a single-file or two-file version of LESS that can be included as an AMD module on the server.

For example, the two-file solution would look like:

lib
-less.js
-less-server.js

On the client I would use:

  require(['less'], function(less) {  ...  });

And on the nodeJS server I could use:

  require(['less-server'], function(less) { ... });

Using them both the same way. Currently, server AMD is slightly messier without having the ability to load everything from one or two files as shown above.

@lukeapage
Copy link
Member Author

It sounds like a step in the right direction.. presumably use requireJS to create the single files?

As I said before though its not high on the list of priorities and I think it would be good to do some refactoring of the client/browser responsibilities to make things clearer, which would facilitate something like this.

@guybedford
Copy link
Contributor

One refactoring could be to use RequireJS to do such a build itself. But currently the module format is very much common-js based so that may take a little work to ensure it optimizes correctly.

My suggestion would be these two build variations based on the environments:

  • less.js for the browser
  • less-server.js containing both node and rhino support for the server

Both the browser and server versions could support amd if it detects it in the environment, otherwise defaulting to the current environment system.

I probably should have started with this high level discussion instead of dumping the code, but the changes in #868 and the code block above go some way to achieving this.

@tlindig
Copy link

tlindig commented Jun 6, 2013

@guybedford
You Said: "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."

I can not understand, what the advantage of this would be, but a see some major disadvantages.

A fix name, make a browser side AMD-like support much more easier. I have written such a simple define method, but this need the id. This id allow, that all script loaded with regular script tags. no ajax is needed. Without the moduleId, you can not do this, because you have to figure out a id based on the file name of a script before the define method is called. and you have to do it for every script.

The old way was 100% AMD conform and worked very well. So please undo this change.

@guybedford
Copy link
Contributor

@tlindig see here for more info - https://github.com/jrburke/requirejs/wiki/Updating-existing-libraries#wiki-anon

I understand that named defines are easier in some cases, but this ease of use comes at quite a high cost.

Portability is the key reason for using anonymous defines. The point being that the exact name of the module can be changed. In my case, writing the require-less plugin for RequireJS allows less to be required with the syntax:

  require(['less!some-file'], ...);

This is only possible since the less compiler uses an anonymous define and hasn't already taken the less moduleID.

There are quite a few issues now with jQuery's use of a named define, one can't run jQuery 2.0 and a legacy version together in the same project for example because of this without forking out this named define.

If you are writing your own loader, then use the URL to infer the moduleID. If you have simply defined window.define and are using a script tag to load less, then ensure that your define.amd is not present and it should be skipped?

@ktiedt
Copy link

ktiedt commented Mar 24, 2014

Trying to get an idea of what the status is on this? It seems there have been pull requests/commits and then they have been removed? 2 years is an awfully long delay in porting something to a format that isnt that difficult to rewrite to... I am mainly asking because of the requirejs-less plugin currently still includes a modified less.js file because of this and being able to easily update that w/o rewriting it everytime would be amazing. -- Thanks

@guybedford
Copy link
Contributor

Note - I've just updated the version in RequireLESS and it didn't need any of the above hacks for me anymore. Working great.

@ktiedt
Copy link

ktiedt commented Mar 27, 2014

@guybedford awesome news! With that being said, is that previously not working the only reason a prebuillt version of less.js being shipped with RequireLESS?

@guybedford
Copy link
Contributor

@ktiedt I've created an issue here - guybedford/require-less#62

@lukeapage
Copy link
Member Author

@guybedford please could you take a look at my changes here - #1902
I think I've been consistent.. and let me know if this will likely cause any problems or if its exactly what you want.

@lukeapage
Copy link
Member Author

less.js has been completely refactored and now uses browsersify. This means that everything can be required to make a specific browser build. I think that satisfies this issue now and we should perhaps handle anything more specific in a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants