define should be anonymous

[graingert]

No description provided.

[mikesamuel]

Why is an anonymous define preferable?

[graingert]

@mikesamuel define should either be anonymous or match the npm+bower name. Because these are now different you need to use anonymous define.

[kevin-brown]

Why is an anonymous define preferable?

It makes it very difficult to include a package in your build system if it's not an anonymous define. When it's named, your build system either needs to detect that (it usually doesn't) and alias local imports for you, or you need to explicitly map the defined name to the file location, so you that you can still import it.

It actually causes enough issues that there's a dedicated page for RequireJS explaining how to use jQuery with it.

http://requirejs.org/docs/jquery.html

[mikesamuel]

Ok. I see

So to reiterate, you will likely get an error if you refer to jQuery with another module name, like 'lib/jquery'. This example will not work:

    // THIS WILL NOT WORK
    define(['lib/jquery'], function ($) {...});

It will not work, since jQuery registers itself with the name of 'jquery' and not 'lib/jquery'. In general, explicitly naming modules in the define() call are discouraged, but jQuery has some special constraints.

[mikesamuel]

This looks good to me. @amroamroamro has a number of large patches in flight so I'm not going to merge yet. It's much easier to redo a one liner manually than to merge one into a multi-file multi-commit PR.

[graingert]

@mikesamuel that's definitionally false. You can transform the problem of merge A -> B, into merge B -> A with simple operations: squash(revert B, apply A, merge B -> A, apply B);

[amroamroamro]

@mikesamuel patches 3 and 4 (demos and tests) are ready to merge if there are no further reviews/comments.
I'm ok if you want to merge this one too next, it won't conflict much. I usually git rebase before I submit pull requests anyway.

[amroamroamro]

Now I'm thinking out loud, but do we need to keep the AMD define()? From what I understand (I'm no expert, so correct if I'm wrong), bundlers and transforming tools have largely made this obsolete (browserify, webpack, rollup, etc.), so perhaps it's not as important anymore...

What is the convention these days regarding module loaders, bundlers, and such? I don't want to propose any major changes for now, just to get an idea.

Also keep in mind that the source is fed to closure-compiler, so these solutions would have to be compatible with that, which can be tricky at times in ADVANCED compilation mode.

[amroamroamro]

@graingert I remembered, you need to also update the definition in amd-externs.js for this PR, otherwise you'll get a warning from closure-compiler.

>> /src/prettify.js:1738: WARNING - Function define: called with 2 argument(s). Function requires at least 3 argument(s) and no more than 3 argument(s).
>>     define([], function () {
>>     ^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> /src/prettify.js:1738: WARNING - actual parameter 1 of define does not match formal parameter
>> found   : Array
>> required: string
>>     define([], function () {
>>            ^^
>>
>> /src/prettify.js:1738: WARNING - actual parameter 2 of define does not match formal parameter
>> found   : function (): ?
>> required: (Array<string>|null)
>>     define([], function () {
>>                ^^^^^^^^^^^^^

---

In fact, the first line var define = win['define']; should be dropped, it knows about define as an external symbol since we pass the option --externs='tools/closure-compiler/amd-externs.js' to the compiler.

---

One more thing, you must not manually edit src/prettify.js, you changes should go into js-modules/prettify.js instead. The former is generated from the latter.

[mikesamuel]

@mikesamuel that's definitionally false. You can transform the problem of merge A -> B, into merge B -> A with simple operations: squash(revert B, apply A, merge B -> A, apply B);

I'm not up on DARCS theory of patches, but I do know that if I try to apply a single line patch after merging a change that changes indentation of the block containing it, then I have to do more work.

[mikesamuel]

@mikesamuel patches 3 and 4 (demos and tests) are ready to merge if there are no further reviews/comments.
I'm ok if you want to merge this one too next, it won't conflict much. I usually git rebase before I submit pull requests anyway.

I'd marked them approved through the code review interface. I'm happy to do the merge though.

[amroamroamro]

@mikesamuel oh noted. I wasn't sure if you wanted me to do the merge, thanks.

[graingert]

@mikesamuel glorious. can we get a .mjs es-module next?