Require using AMD broken in 10.0.0

[jlchereau]

Loading via SystemJS in AMD used to work with v9.18.1. Broken in v10:

system.js:4 Uncaught (in promise) ReferenceError: hljs is not defined
    at eval (highlight.js:1566)
    at eval (highlight.js:6322)
    at eval (<anonymous>)
    at Qe (system.js:4)
    at system.js:4
    at async Promise.all (:63342/Kidoju.Widgets/src/js/widgets/index 10)

hljs is undefined when calling L1566

hljs.registerLanguage('apache', function () {
   ...
}());

In System.config.js, I have:

packages: {
    'src/js': {
        defaultExtension: 'js',
        meta: {
           '*.js': {
                babelOptions: {
                    es2015: false,
                    stage2: false,
                    stage3: false,
                },
                format: 'amd',
            },
            '*.es6': {
                format: 'esm',
            },
            '*.jsx': {
                format: 'esm',
            },
            '*.mjs': {
                format: 'esm',
            },
         },
      },
 },

A workaround for now is to add an exception in meta:

 'vendor/highlight/highlight.js': {
      // Note: AMD broken as of v10
      format: 'global',
},

[joshgoebel]

I'm not sure we ever were truly AMD compatible, the old init just did this:

  globalObject.hljs = factory({});

    // Finally register the global hljs with AMD.
    if(typeof define === 'function' && define.amd) {
      define([], function() {
        return globalObject.hljs;
      });
    }

IE, define a global, and then tell AMD to use that global... the new build system actually is actually behaving better (AMD wise) but the problem is now when we're trying to register the language modules there is no top-level hljs yet likely because I would assume AMD isn't (ever?) going to make one for us?

The language modules have always registered themselves via a top-level hljs global object.

So the behavior is different, but I'm not sure that's actually a problem - since it was "all globals all along" so far as I can tell. I'm not sure what the right answer is here because I don't think what we're doing before is actually correct... so perhaps someone with more knowledge can weigh in here.

[joshgoebel]

Perhaps we should just remove our AMD support? Perhaps someone super familiar with AMD could explain what we'd have to do with the language modules to make them truly AMD compatible (hard to say if that's viable without knowing what would be required)... just going back to how we had it before seems quite broken to me.

Do all the language modules need UMD wrappers? That would be annoying.

Some language modules also have their own built-in dependencies and expect to be loaded in the proper sequence... they would not do so well if the order were randomized, etc. (the build process takes care of this by inserting the registrations in the proper order in the final web build product).

[jlchereau]

I think the problem is in the structure of the build output file which is:

(function (global, factory) {
  typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
  typeof define === 'function' && define.amd ? define(factory) :
  (global = global || self, global.hljs = factory());
}(this, (function () { 'use strict';
  const HLJS = function(hljs) {
    ....
  }
  var highlight = HLJS({});
  return highlight;
})));

hljs.registerLanguage('apache', function () { // <-- hljs is undefined when using AMD
  ...
}());

After removing all hljs.registerLanguage, AMD loading works fine and returns the hljs object to the loader.

[joshgoebel]

Yes, I understand exactly why it is failing. The new AMD code fails to setup a global object (as it did before), so this now fails.

I'm less sure of the path forward.

This output is how the build process works (and always has). Languages are "modules" that are added AFTER the core library is setup by calling registerLanguage. This is also currently a requirement of the website builder/download tool too... the languages are just appeneded to the end of the core library.

[joshgoebel]

After removing all hljs.registerLanguage, AMD loading works fine and returns the hljs object to the loader.

And now you have no languages, so the highlighter won't work. :)

[joshgoebel]

From a different angle: What benefits do you gain by using AMD here vs having to declare it "global"?

[jlchereau]

I know but the problem is not with your UMD wrapper which is fine.

[joshgoebel]

I know but the problem is not with your UMD wrapper which is fine.

Well it depends how you define the problem. :-) You could also say the wrapper is EXACTLY the problem. The old wrapper setup a global, the new wrapper does not - and as a result it fails.

@egor-rogov Any thoughts here?

[jlchereau]

From my end, I can certainly use a workaround either with SystemJS (development) or Webpack (production). I'll always find a solution. I just thought it was nice to report an issue.