Remark on initModule

[Ayms]

Hi again,

In older versions, you were checking window.

I think it was better because now it's not possible without hooking inside node.js to run the same code inside a browser and inside node.js (when you don't want to use node.js's modules, it can look strange but this is what I am doing).

To be clear: it seems more clean to define a window var on a server environment if you want to keep the code as it is inside the browser rather than destroying process properties, module.exports, etc to do so, which have of course side effects on other parts of the code that are using node.

Regards,

Aymeric

[dlongley]

@Ayms, I'm having difficulty understanding what you're suggesting. Could you type up two quick examples of code so they could be compared?

I also don't follow when you say: "destroying process properties, module.exports, etc". I don't think anything is "destroyed" by the current code, but perhaps I'm misunderstanding what you mean.

[Ayms]

Yes:

My code = code for browsers (client) code for node (server) code for both (browsers and node) code for browsers (client) integrating forge's code (asn1.js, pki.js, etc), but I want this code to be valid when it runs into node (server) without calling the modules (asn1.js, pki.js, etc), because I might have modified them and don't want to maintain them in two places.

Then with the current implementation, I have to do (not theoretical, I synced the latest master and tested it):

process.nextTick=null; process.versions=null; module.exports=null;

Which might/will screw up the rest of the code dedicated to node in 'My code'.

It would be more simple that the check occurs on 'window' and/or 'window.xxx' because you can easily adapt on node/server side environment defining a window var and associated properties rather than modifying important properties on the server side.

[dlongley]

Are you saying that you want the client-side implementation of forge to run on the server or vice-versa? If so, why would you want to do that, what's wrong with using the proper implementation? If you've made modifications to them, I don't understand why you'd need to maintain them in two different places or what those two "places" might be. Are you serving the files from different locations? Why can't you just make sure the modifications always run regardless of the environment? Sorry for so many questions, but it's still unclear to me what's going on here -- can you be very specific with examples?

I want this code to be valid when it runs into node (server) without calling the modules

I'm sorry, but I'm still not sure what this means. I get the general idea that you're saying that you've got some code that you've modified that you'd like to run in two places but it won't run properly because of the boilerplate we have for loading forge in various environments, but I can't figure out the specifics. I also don't understand why you'd need to do this:

process.nextTick=null;
process.versions=null;
module.exports=null;

Which code, specifically, won't run without you doing that? Can you provide a snippet of JS? Does it fail on the client or on the server? (Please provide a quick block of JS so we can better understand the specific problem and maybe offer an alternative solution).

The boilerplate is working well for other people who are running forge on both the client and the server. I don't understand what involving window solves -- but if you could provide two snippets of JS showing how something would work in one case (with window) and not work in another (without window) and indicate where you're executing this code that would be helpful. It sounds like you're trying to run the client-implementation on the server but I don't understand why. There may be an alternative solution to this issue, like putting appropriate environment checks in your own code.

[Ayms]

Please see here : http://www.peersm.com/test.html , the js file is an extract of code running both in browsers and in node.

For different reasons the code has to be a monolithic block, so the necessary forge files are not loaded but included in the code, then there is not a place where files are maintained and loaded by the browsers and/or node.

Some of them are slightly modified, for different reasons too.

With the current implementation, if I don't want to modify initModule in each module and want this to work inside browsers and inside node, I have to do something like the modifications that you see at the begining.

Knowing that this is just a small portion of code extracted from something working inside browsers and inside node, these modifications are impacting the part of the code that is really using node.

That's why I think it's better to check browser's properties first and then node properties after when needed.

[dlongley]

Ok, I'll take a look at your code when I get a chance, but something to keep in mind is that forge runs in more than just two environments (browser and node.js), it also runs in Titanium Appcelerator, Adobe ExtendScript, and in pseudo-browser environments. If any changes are necessary, we have to ensure not to break those environments. I'm hopeful that there's another solution here, though I'll have a better idea once I get to look at the code you've linked to.

[dlongley]

@Ayms, I still don't understand why you're making these changes at all. Why don't you just let the code run like it normally would in node? Why are you trying to run the client-side implementation in node?

To be specific, why are you defining window when it isn't defined? This will cause node to use the client-side implementation for things like the PRNG -- when node.js has a built-in, secure, native version of this. Why wouldn't you want to leverage the native implementation when it's available? I don't understand why you're doing what you're doing -- it seems like a mistake to me. Why, specifically, do you want to run the slower, pure JS implementation of certain parts of forge when node.js provides native APIs for those features? Are you aware that that's what you're doing by changing this code the way that you have?

If you remove that extra code block at the top that forces node.js to use the less efficient (and potentially less secure) implementation, are you getting a specific error that we can help you with? You shouldn't be getting any errors by doing that -- it should just cause the code to use the best available implementation of the forge features you need based on the environment it's running in.

[dlongley]

@Ayms, are you specifically trying to run the pure-JS implementation in node.js? If so, why? Why wouldn't you prefer the faster, potentially more secure, (and still open source) native implementation?

[Ayms]

@dlongley Yes, and I know what I am doing, including prng (I did not say I was using forge's one), security, performances & co

The complete code is quite complex and works inside browsers and node, trying to use the best of each, it's not so obvious to do.

I don't want the browsers to fetch tons of scripts, for different reasons, so I am including external modules like forge directly in the code, minified.

But it happens that I have to modify them, so right now I don't want to maintain them in the code and in the forge repo (+your updates).

Then for some things that do not impact performances/security I run the pure-JS code in node.

Maybe the final version will be as you suggest but right now I prefer not to add useless complexity.

Anyway, I know forge works on other platforms too, what I am suggesting here is to have a means to run forge on the platforms as pure-JS without having to hook platforms properties.

That's a choice that should be allowed, pure-JS can be faster for example than platforms modules in some cases.

[Ayms]

I would suggest to modify:

if(typeof module === 'object' && module.exports)

by something like:

if((typeof module === 'object' && module.exports)&&(typeof forgeJS ==='undefined'))

Then if forgeJS exists, we go for the pure js version whatever the platform is.

[Ayms]

... not very good what I suggested, have to think about it...

[dlongley]

@Ayms, what I'll probably do is see if I can add a flag like usePureJS to the global forge object (or to one you pass when requiring forge) that the modules will check when making decisions about which implementation to provide. If that works out, then you can just create a global forge object with that flag set to true before you include everything else.

Anything else is probably not the right approach -- this would directly address the behavior you want and might be useful in testing.

[Ayms]

Probably, I don't think there should be DOM or other dependencies but maybe you could combine the flag with a test on postMessage so the pureJS implementation is provided automatically for browsers/workers.

[dlongley]

@Ayms, I've added a 'disableNativeCode' flag that you can set to true at the top of the file you linked to that should produce the result you want. It looks like you'll still need to set module.exports = null because you're trying to avoid requiring any files at all in node.js, but that shouldn't be a problem.

Let me know if this works for you.

[Ayms]

I don't know the impact of killing module.exports in node.

If I am correct the "boot" module for initModule is the same in every module.

So, why not defining function boot(deps) {...} in something like boot.js, then:

xyz.js:

//include boot.js function initModule (forge) { ... ... boot(deps); }

and in boot.js (your forge.js's modifs modified as follow):

if(typeof forge === 'undefined') {
      forge = {disableNativeCode:postMessage||null};
}
if((typeof module === 'object' && module.exports)&&(!forge.disableNativeCode)) {
    nodeDefine = function(ids, factory) {
      factory(require, module);
   };
}
  // <script>
  else {
    initModule(forge);
  }

[dlongley]

Setting module.exports to null shouldn't be a problem in node. It just means you're not exporting anything (or more specifically, you're exporting null).

Unfortunately, it doesn't make sense to do this:

if((typeof module === 'object' && module.exports)&&(!forge.disableNativeCode)) {
...

Because that flag has to do with excluding native code, it is not about messing with the module loader. Ultimately, what you're doing with your code in node.js is non-standard. I'm ok with coding another solution if it makes sense to be used by others, but I don't want to add some custom code that either complicates things for everyone else or adds bloat to the library just to cover a non-standard approach that has a recommended alternative. I'm not sure the above proposal meets both of those conditions.

I also don't want to encourage people to use forge in a way that deviates from common practice. You should be requiring the modules like usual in node.js, not concatenating them all together and then loading a single file. There are various projects out there that can help you share code between the browser and node.js if you want to serve a single file to the browser (eg: https://github.com/substack/node-browserify). I understand that you don't want to have to do maintenance in more than one place -- and you shouldn't have to. Other projects exist to help you accomplish exactly that and they are the recommended way to proceed here. You shouldn't have to break convention on the node end to accomplish your goals. You also don't want to harm future-extensiblity and reuse of your own code.

[Ayms]

Thanks but I know all this already. I find it strange to have the "boot" code duplicated x-times in all modules, and I find it strange to have the forge var instantiated as a global (which does not follow at all best js practices and strict mode)

I am not asking this for myself because I already modified it the way I liked, I think something like a boot.js could be good because you can then clearly decide what you want to do.

Maybe I was not clear: forge is part of the code, I don't want to use node for this one (because there is no point doing such), but I do for other parts, so I am not going to modify module.exports.

There is nothing in my proposal that deviates from common practices (except that I did not correct the global var pb), that's a simplification that makes it easier to decide what people want to do without modifying n files.

And forge's goal is for browsers, so node/platforms concerns seem a detail.

And from the browser perspective, if I am concatenating the code, there are very good reasons for that beside maintenance.

I think you should consider it again.

[dlongley]

I find it strange to have the "boot" code duplicated x-times in all modules, and I find it strange to have the forge var instantiated as a global (which does not follow at all best js practices and strict mode).

Forge is only instantiated as a global if it is loaded without any module loader. If you load it using any other method, you can instantiate it differently. I believe this to be common practice. The reason why the boot code is duplicated is for the same reason: common practice. It would be a break from convention to require two different files to be loaded every time you just wanted to load a single module. Furthermore, it would force you to use a script tag to load the "boot loader" so you couldn't just use the module loader you're already using to load everything else. That too, I believe, would be a break from common practice and may require annoying infrastructure changes for other people.

Maybe I was not clear: forge is part of the code, I don't want to use node for this one (because there is no point doing such), but I do for other parts, so I am not going to modify module.exports.

If you're including that code in a module in node.js, there's no problem with setting module.exports to null. If you need to export an API from that module, there's still no problem, just set module.exports after running the forge code. If you are running the code in node.js outside of a module, setting module.exports to null is fine. If you're running the code in the browser without a module loader (using script tags), then setting module.exports to null is not an issue. Under which circumstance is it an issue for you?

There is nothing in my proposal that deviates from common practices

I think that's inaccurate. Here are two problems with your proposal:

1. Requiring someone using a module loader (in browser) to include another script tag (the boot loader) is a deviation from typical use; they would instead just expect to include whatever module they wanted as usual without the use of a script tag. The question becomes: "If we're supporting AMD, why aren't we supporting AMD?" Because that's how it looks when you have to start using script tags to make things work.
2. Requiring someone to require() a boot loader in node.js instead of just requiring the module they wanted -- and then needing to inject what was loaded from the boot loader into the other module so it can properly load itself is clearly a deviation from common practice. I don't see a good reason to do this.

If these can be avoided by a change to the proposal and there is not unnecessary bloat added to the library (your problem could just as easily be solved using a common tool), then I will definitely reconsider it. I'm not dismissing the proposal out-of-hand, I'm saying that I think it would affect other people in a negative way. I'm saying that I think more people would think that it would be the non-preferred solution to this problem, instead I think they would tend to agree that you should use one of the common "browserify-like" tools for your use case.

And forge's goal is for browsers, so node/platforms concerns seem a detail.

Right now, I think we're covering both in a natural way. My understanding is that what you're trying to do is somewhat unusual, or rather, there are alternative approaches you could take that would not require forge to be changed whilst still accomplishing everything you want.

And from the browser perspective, if I am concatenating the code, there are very good reasons for that beside maintenance.

You should certainly be able to concatenate code. I don't think that you're limited from doing that right now. I just think that the problems you're running into could be avoided using a common tool chain rather than a custom solution that deviates from what most other people do, and therefore the changes you've proposed to accommodate your use case would cause the forge library to deviate from what most other people expect.

I think you should consider it again.

Again, I'm not rejecting your proposal out-of-hand, rather, I need to ensure that forge is behaving in a way that most people would expect. I don't think that your proposal would accomplish that.

To reiterate:

1. I don't want to tell people using AMD that they now have to use a script tag to load a boot loader before they can load supposedly-AMD-style forge modules. The forge modules are not actually complying with AMD if that is necessary. I also don't want to tell them to write their own boot loader.
2. I don't to tell people using forge in node.js that they have to require a boot loader before they can require any other forge modules and that they must pass that boot loader to an initialization function for the other modules (or write their own boot loader).

[dlongley]

I'm cc'ing some others who may have opinions on the changes proposed here: @msporny, @davidlehn, @hiddentao, @cadorn

[hiddentao]

I think I agree with @dlongley on this one. Having the initModule boilerplate in every forge module is just common practice. Additionally, if you're Gzipping your concatenated browser code these repetitive sections should get factored out and compressed quite efficiently anyway.

As for the using a boot loader in order to inject code into modules it just sounds like extra unnecessary work. I'd rather keep the module exporting mechanisms as they are.

[Ayms]

@dlongley @hiddentao OK, if I understand correctly what you don't like is the idea of a boot.js file, which I don't like either.

What I would do is to give the dependencies for each module and provide the code for the loader separately, so people using forge know how to run it.

But you don't like the idea too as far as I understand, common practices do not mean best practices, AMD is here today because the previous common practices loading methods were not good ending up with globals, that's one of the reason why I am concatenating the code: I really don't want inside the browsers any global to escape the code wrapper, and I don't want to use tons of code of external libs to achieve this simple goal.

But if I understand correctly again, forge user needs to do something like var forge={} before loading modules, if not forge becomes a global even with AMD.

So if they have to do so, they could rather do by themselves:

function load(xx) { ... forge's advised module loader ... } var forge={}; load('pki.js')

Common practices or not, I really find it strange to have the module loader in each module, I would really separate it as proposed above.

For all the external libs I am dealing with, the problem is always the same: you need to modify what the author has foresee to load the code, the annoying side effect is that you need to make these modifications each time you update the code, while it would be much more simpler for the authors to provide the code and let you instantiate it the way you like providing the necessary information.

And instantiating a global without using var is not very esthetical and forbidden.

[dlongley]

But you don't like the idea too as far as I understand, common practices do not mean best practices... Common practices or not, I really find it strange to have the module loader in each module, I would really separate it as proposed above.

I'm not asserting that common practices are the most efficient or clever way to accomplish something. They aren't "best" in that way, that's true. But they are "best" in that they are the most compatible way to do something with respect to the community. The most likely outcome, should we deviate from common practice, is that people just won't use forge. It will annoy people who have come to expect things to operate in a certain way, even if there's an alternative with other advantages. Forge is not currently in a position to spark a revolution in how client-side modules are loaded.

But if I understand correctly again, forge user needs to do something like var forge={} before loading modules, if not forge becomes a global even with AMD.

No, by following the AMD API/model, forge does not become a global. The way AMD (eg: RequireJS) works is that you pass in a variable to the define function that you'd like to hold the API to the module you're loading. Then the forge API is defined on that var. This prevents forge from being established as a global var.

The only time forge is a global var is if you're using script tags and you haven't defined forge before loading the forge modules. If you're using script tags and you don't do that, something has to get into the global space otherwise there's no way for you to interact with what you've loaded.

[cadorn]

The loader is not in each module. Each module only includes a shim to hook into the various module environments.

@Ayms I understand your frustration and have run into these kind of cases before. To that end I am building a toolchain that can take any wrapped module and produce a standard non-leaking module that can be loaded with a non-polluting module loader. Its a work in progress but if you are interested more longer term you can find it here: https://github.com/pinf-it/pinf-it-bundler You can find the module formats it supports here: https://github.com/pinf-it/pinf-it-bundler/tree/master/test/assets/modules

The point I have come to realize is that in the absence of a module format that works everywhere it is futile to try and convince project authors to change the way they are loading modules if their current solution works for their current use-cases. If integrating various third party code you are better off using tooling that sniffs out the dependency tree and use your own custom loader that can replace modules. This may not be elegant but it solves the problem today and is the goal of the tooling I am building. Unfortunately its not quite ready for real use yet.

In regards to your problem with forge. I suggest you send a a minimal PR that solves your problem in the most non-intrusive way so that all existing use-cases continue to function without modification.

FYI; Longer term I believe everyone will be coding without boilerplate as the tooling to manipulate modules on the fly to adapt to various loaders will become ubiquitous at all levels of the toolchain. Once that happens you have to rely on tooling to manipulate your dependency tree; might as well start now.

[Ayms]

@dlongley right for AMD, I did not read it correctly. What I mean too is that forge is not really a "common" module, it does involve crypto, certificates, ssl/tls, things that need to be handled carefully, for example people loading it inside browsers using script tags and then ending up with a global forge var looks to me a kind of amateurism. Separating the loader is not really a revolution. Maybe we can let the issue open and see if a better solution shows up in the future.

@cadorn, thanks for the links, indeed modules loaders are tricky, all different and next will come the standard ES6 modules. Right now I don't have a minimal non intrusive solution, If I were not to concatenate the code and load it from the outside I would probably use (correctly) xhr + eval

[dlongley]

Closing after updates via #456.