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

AMD dependencies on cjs/shimmed modules cause 'Module not already loaded loading "process" from "undefined"' error. #942

Closed
sberney opened this issue Nov 30, 2015 · 6 comments
Labels

Comments

@sberney
Copy link

sberney commented Nov 30, 2015

I've created a simplified project that only imports velocity.ui (which supports use as an amd, cjs, or global module and is distributed with velocity). Velocity itself is a jQuery shim -- I'm marking it as a cjs module. Adding the following to my config.js file allows velocity.ui to be imported with its dependency on velocity resolved (but...):

meta: { "velocity": { "format": "cjs" }, "velocity/velocity": { "format": "cjs" }, "velocity/velocity.ui": { "format": "cjs", "deps": ["velocity"] } },

But, if I instead use
...
"velocity/velocity.ui": { "format": "amd",
...

I get 'Uncaught Error: Module not already loaded loading "process" from "undefined".'

It seems to me that this should work, given that velocity.ui ostensibly supports amd loading. This problem has recurred a couple times in migration of ~12,000 lines of js from requirejs to systemjs, so I'm hoping to get this resolved: I have some homebrewed amd modules whose cjs/shim dependencies aren't being resolved. (If I change define to require in the amd files, some of the error messages go away, which also confuses me.) I've attached the simplified project (as text files, sorry).

config.txt
index.txt
package.txt

@guybedford
Copy link
Member

What module is loading require('process') in your environment here in the first place?

@sberney
Copy link
Author

sberney commented Dec 1, 2015

You're right to wonder that, because as far as I can tell, neither velocity.js nor velocity.ui.js use process. However, when I do the following, and look into the velocity source that gets downloaded, both modules are wrapped in function(process) { ... }(require('process')).

To reproduce, only the following is necessary (I've done this a several times to be sure):

  • jspm init
  • accept default settings
  • jspm install velocity=github:julianshapiro/velocity@1.2.2 -o "{ 'registry': 'npm' }"
  • add meta configuration below to config.js
  • copy index.html to base directory (contents included below)

from config.js:

meta: {
    "velocity": {
      "format": "cjs"
    },
    "velocity/velocity": {
      "format": "cjs"
    },
    "velocity/velocity.ui": {
      "format": "amd",
      "deps": ["velocity"]
    }
  },

index.html:

<html>
  <head>
    <script src="/jspm_packages/system.js"></script>
    <script src="/config.js"></script>

    <script>
      window.require = System.amdRequire;
      window.define = System.amdDefine;
      System.import('velocity/velocity.ui').then(function (Velocity) {
        console.log('imported velocity.ui');
      });
    </script>
  </head>
  <body>
    Hi there!
  </body>
</html>

@sberney
Copy link
Author

sberney commented Dec 2, 2015

Could this have something to do with "{ 'registry': 'npm' }"? I am under the impression that this registry setting is more concerned with velocity's dependencies and their configuration, not with modifying the actual code or obtaining the library from a different source. I still get velocity from github when I use the npm registry, right?

In case you are astute, you might notice that the velocity.js and velocity.ui.js files I linked to above are version 1.2.3, and I was installing 1.2.2: I checked this doesn't make a difference. And performing a fresh install without the registry option doesn't prevent the wrapping either. Am I misunderstanding something?

@guybedford
Copy link
Member

Yes exactly, when installing from npm, we treat all modules as CommonJS, which involves adding the process dependency as a CommonJS wrapper if the process global is used at all.

If you then alter the format to AMD, the require('process') wrapper statement will fail. What is the reason for adding the registry: npm here?

@sberney
Copy link
Author

sberney commented Dec 3, 2015

Thanks for the response. I am using registry: npm because:

  • Installing from github without any override skips installing dependencies (as I understand, this is the default github:* behavior). In the case of velocity, the dependency is jquery.
  • Installing from jspm in this instance proxies to github, does not warn about skipping dependencies, and skips them anyway. After running jspm install velocity=velocity@1.2.3 there is still no copy of jquery downloaded, even though it's specified in velocity's package.json and bower.json. For this particular library, it turns out some of jquery's functionality is embedded, however velocity does depend on jquery.
  • One of the first things I did was install and configure jspm-bower-endpoint. But I've noticed that sometimes bower packages don't seem to get installed, and/or don't seem to get their dependencies downloaded. For example, jspm install bower:backbone.marionette@2.3.2 will create a nearly* empty directory bower/backbone.marionette@2.3.2 (*: it contains .jspm-hash); including a lack of dependencies. As for velocity, I just checked, when I use bower in this specific case of velocity@1.2.3, jquery does get downloaded.

I had enough of these issues that I upgraded Marionette and a number of other libraries (a good thing to do, but takes time and effort), and avoid using the bower endpoint anytime there's a choice.

In general, I'm installing npm packages because it seems like the alternative is github, and using github currently requires me to keep track of the third party's dependencies; adding them to my config.js.

Just checking, sorry if it seems repetitive: when I use registry: npm, SystemJS is still preparing code à la node from github, right? It doesn't look in the package.json and then run off and download the npm tarball?

Please correct me if I am misunderstanding: the jspm registry currently just delegates to a registry preferred for the given package.

So if the default jspm registry entry maps to npm (as it does sometimes: for simple example: any-promise-es6), it's no different than if I installed with npm:any-promise-es6. Which is in turn, no different from a jspm point of view than installing from github while using the registry: npm attribute: in each instance, the library is downloaded and wrapped.

On the topic of this error message: velocity has only a dependency on jquery. jQuery has no dependencies. I said

"And performing a fresh install without the registry option doesn't prevent the wrapping either."

And it certainly didn't yesterday. Today I've reinstalled the global jspm, and have made sure to install a local copy into each project. Now, when I create a fresh project, it seems that Velocity is no longer wrapped (phew). This is a mystery to me.

I noticed that transpiler? yes and, babel leads to having a copy of process. Before jspm, no package needed process and no copy of it existed. I have upgraded some packages, so it's not beyond possibility that some package now relies on it. At some point jspm added it as an explicit dependency in config.js for one of the Marionette/Backbone/Wreqr/Babysitter/jQuery/Underscore chain, although I currently can't reproduce the circumstances for it to do this.

Correct me if I'm wrong: Only explicit dependencies of a package installed with jspm are candidates for wrapping that package. If process was a dependency of something else not velocity, velocity should never be wrapped for process.

So velocity should never have been wrapped for process.

Intelligent handling of global libraries is actually a gigantic issue that I need a solution for. Experimentally, I found out SystemJS loads global libraries globally. So, do npm dependencies need to be wrapped for Node and that's the reason and only reason for the wrapping? Apologies, because relatively, I am a newcomer to the javascript toolchain.

This last part should nearly certainly be a separate question,

but it's tangentially related: For global libraries, SystemJS doesn't provide any way to "un-global" it, does it? I can't think of a foolproof way this could ever be done (but it would be amazing and fix everything in the universe: that is, to prevent libraries from writing to the real window and let them write to window.SystemJSManagesThisGlobalSpace).

Barring the ability to encapsulate, how does SystemJS create window.someAttribute?: Highcharts is my # 1 offender. It loads itself globally, and refuses to load when window.Highcharts already exists. I would like the ability to load two versions of Highcharts "at once"... If not on the same page, one after another. I've been playing around with

System.delete(System.normalizeSync('highcharts-v1'));
delete window['Highcharts'] && delete window['HighchartsAdapter'];

But, if I want it back, System.import('highcharts-v1'); doesn't recreate the global variable, which seems like it must involve some inner workings of SystemJS.

@guybedford
Copy link
Member

So if the default jspm registry entry maps to npm (as it does sometimes: for simple example: any-promise-es6), it's no different than if I installed with npm:any-promise-es6. Which is in turn, no different from a jspm point of view than installing from github while using the registry: npm attribute: in each instance, the library is downloaded and wrapped.

Yes exactly.

The process wrapping is done for any module on registry: 'npm' that contains a reference to process.

I was actually just working this past week on the upcoming jspm 0.17 implementation of this conversion process, so that we don't do these types of wrappings for other module formats and avoid these issues by first doing a full module format detection for every module before handling the conversion. At the moment registry: 'npm' will always force every module to be CommonJS and do these wrappings, which is I think what is limiting here.

SystemJS could in theory stop globals from writing to the global. SystemJS carefully controls the globals available to global scripts so that multi version globals are supported. We do this by managing the globals on the window object before the execution of each global. But the edge case is non synchronous reads to window.globalName, say via setTimeout(function() { window.jQuery /* must exist here, so we can't remove window.jQuery */ }, 1000).

We could potentially enable this full encapsulation quite easily with a flag. I've created #960 which would solve your Highcharts issue I believe.

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

No branches or pull requests

2 participants