Switch to CommonJS - #357

I see that a lot of files changed here (as you removed all the boilerplate) -- but that makes it difficult to tell if you provided any sort of build system to support different module loaders. Previously, forge could be loaded via CommonJS or AMD, for example. Is that still the case with your changes?

What we'd really like to see is a build system that can generate the boilerplate that people want to use for the module loading systems they want to use (for which there are several different approaches).

I have made a couple a projects that show how to use different popular module loaders:

• https://github.com/ysangkok/forge-webpack-example
• https://github.com/ysangkok/forge-requirejs-example
• https://github.com/ysangkok/forge-browserify-example
• https://github.com/ysangkok/forge-rollup-example

I can test with other systems like JSPM if you desire.

So prior to this change forge had the following attributes:

• Annoying boilerplate and problems with static analysis, "use strict" and other issues with certain loaders (Webpack, browserify, etc.)
• Ability to load via AMD or CommonJS.
• Ability to load individual modules with AMD and CommonJS (not just the entire forge package).
• Ability to generate an optimized, minified file using the requirejs optimizer.

Can we have automated tests that demonstrate all of the positive things above still working? I see you've got some examples above that can be run manually, which is ok, but automated would be better. Even if we can't get fully automated, can we make sure the tests cover all of the above? Have you tested the minification script, does that still work?

If we can switch everything over to CommonJS and ensure the build tools (as needed) can do everything we were doing before ... and we still pass the test suite (which we're currently not), then we can get this PR integrated. I also want to ensure that anyone pulling down an updated version of forge won't see any breaking changes. Do you know if that is possible with this patch?

We've got a version 0.7.x of forge lined up (nearing completion, but we've not had the time to finish it up yet) -- so if we have to have breaking changes, we'll want to make this change in that version of forge so we do as little disruption as possible. Ideally, we could avoid the breaking changes by having build tools transform the files in whatever way is necessary prior to publishing new packages.

I would suggest using karma for testing that the different setups work in the browser.
You can use

• karma-browserify for ensuring browserify works
• karma-webpack for ensuring webpack works
• karma-requirejs for ensuring requirejs works

Let me know if you need help setting this up.

@dignifiedquire,

Let me know if you need help setting this up.

We need to see it (or similar) as part of this PR (or a companion one) in order to accept this change. I want to ensure we've got backwards compatibility. Otherwise we'll need to roll everything into 0.7.x, which has the drawback of taking longer to push out.

@dlongley I understand, I should have prefixed my comment with, @ysangkok as this was mainly directed at him to help fulfill the requests you made.

@dignifiedquire - no worries :)

@dignifiedquire The browser tests of this fork in /nodejs/test/browser.js are currently using grunt-mocha-phantomjs (It replaces grunt-mocha). I see Karma is just a test runner, how do you see it fitting in with the other parts? Are you proposing adding a Grunt task that calls Karma? I'd like to hear what you have in mind. My plan was to use https://github.com/gruntjs/grunt-contrib-requirejs and run the tests with grunt-mocha-phantomjs like they are now.

@dlongley You say that we are currently not passing the test suite. What tests are you talking about? The ones in nodejs should be working now, but the ones in /tests not yet. Are those the failing tests you talk about?

Using CommonJS, it will be impossible to use the library in the browser without building it. Which module loader would you like me to use for the tests in /tests? I would like to use Webpack, but you seem to prefer RequireJS.

The ability to load using AMD is already tested automatically, in /nodejs/ui/test.js. The ability to load using CommonJS is being tested by all tests in /nodejs/test/ (except the browser test).

Optimization using r.js does not work, I get the error:

Uncaught ReferenceError: module is not defined(anonymous function) @ test.min.js:1

I suppose RequireJS only has incomplete support for CommonJS. But if minification is possible using another project, would it be ok to drop support for the r.js optimizer then?

There should be no problem including individual modules, I can make another example showcasing that if you'd like. But maybe it would be more interesting to not just run all the tests in /nodejs/test/ with one single bundle, but with a minified bundle for each. What do you think? This would allow testing the minification, but I don't know how to get it working with RequireJS, but maybe it would work with Rollup, since it apparently allows for stronger optimization.

I don't think this patch will break any existing usages of Forge. Only people that do not use any kind of build systems or bundlers (loading the individual scripts directly in the head tag) will see permanent breakage. But I don't think many people use Forge like that, as it is a big inconvenience and it is not even mentioned in the README.md that it is possible.

@ysangkok,

The ones in nodejs should be working now, but the ones in /tests not yet. Are those the failing tests you talk about?

I was just talking about the CI ones (those in the nodejs folder). They weren't passing when I wrote that comment, but I see that they are now. Good work!

Using CommonJS, it will be impossible to use the library in the browser without building it.

Do you have any ideas for backwards compatibility? Ideally we'd have a tool that can process the files and output AMD boilerplate so everything loads like it used to for existing AMD users of the library. I want to make sure that if they upgrade forge 0.6.x nothing will break. If you can get something like that into the build system, I think we can figure something out to prevent a significant version bump. There seem to be a lot of tools out there that can autogenerate this sort of thing -- I'd expect we just need to pick the "best" one. Otherwise, we'll be forced to push this off to 0.7.x.

Which module loader would you like me to use for the tests in /tests? I would like to use Webpack, but you seem to prefer RequireJS.

Most of the manual tests in /tests are already using CommonJS because they just run in node. The plan has been to convert that directory into runnable examples (in node) for quite some time, so I wouldn't be too concerned with it. Anything that should be automated that is in that directory needs to be moved to the automated test suite anyway.

I suppose RequireJS only has incomplete support for CommonJS. But if minification is possible using another project, would it be ok to drop support for the r.js optimizer then?

I think we can get the r.js optimizer working again if we can use a tool to process the files and generate the AMD boilerplate. That said, I'm not attached to the r.js optimizer ... my main concern is with people who have integrated their own application minification with forge -- and some of those people will be expecting an AMD interface to work (as well as r.js). I'd really like to make this change without waiting on 0.7.x because it's been a pain point for a long while, so if we can help ensure there aren't disruptions when people upgrade that's all I'm looking for.

@ysangkok,

There should be no problem including individual modules, I can make another example showcasing that if you'd like.

Yes, I think we need that.

But maybe it would be more interesting to not just run all the tests in /nodejs/test/ with one single bundle, but with a minified bundle for each. What do you think? This would allow testing the minification, but I don't know how to get it working with RequireJS, but maybe it would work with Rollup, since it apparently allows for stronger optimization.

Yes -- and this was working previously with RequireJS, but we didn't have automated tests for it.

I don't think this patch will break any existing usages of Forge. Only people that do not use any kind of build systems or bundlers (loading the individual scripts directly in the head tag) will see permanent breakage.

Yeah, those are the people I'm concerned about.

But I don't think many people use Forge like that, as it is a big inconvenience and it is not even mentioned in the README.md that it is possible.

People do use it that way. The future looking goal is this:

1. Write the main files using a simple format (whether that be CommonJS or, eventually, ES6).
2. When publishing new packages, use a build system to generate various boilerplate for different loaders and also provide a minified file. Ideally everyone would just run these tools themselves, but we've found that a number of users of the library have trouble with that.

I'd like this PR to be an incremental step in that direction.

Here is a solution which provides 100% backwards compatibility and convenience: https://github.com/ysangkok/forge-webpack-example/tree/self-contained

I have written an README in that branch explaining what is going on. Please, tell me what you think.

@ysangkok thanksyou for your solution!!! only one think:

The pbkdf2 module exports like the pkcs5:

console.log(forge.pkcs5); 
// { pbkdf2: { pbkdf2: [Function] } }

Yes, we know this is a major issue. I got tasked with trying to figure some of this out and just haven't had the time to finish it.

I took a similar approach to this patch but tried to change less lines of code and keep a similar style to what already exists. Basically loading files adds to the existing APIs in the forge object. Removing the boilerplate and going towards basic CommonJS with minimal other changes did work. Got it to work in node and with webpack and browserify for full and partial builds. And got tests to work across various browsers via karma too. Unfortunately I ended up with too many options and froze on how best to deliver all this.

One problem is how to manage the generated code. Currently you can mostly just run forge in all environments as is. With the CommonJS changes the browser users would need to to generate js and min.js for themselves or we'd have to commit it to a repo. I find it awful to have to generate and check in huge js blobs on every commit or even every release. I'm unsure what to do about that. jQuery uses an external repo for the built code. In the past we've also stayed away from building the code for people due to the theory of not wanting to be responsible for generated code. No one is going to look at huge js or min.js files to see if there are backdoors put in by forge maintainers or minifiers or whatever. In a security minded library this seems like a real concern. Perhaps this is paranoid but we decided to not deal with the issue and let people use tools they trust. But if all browser users will now have trouble using the library without fancy tools, maybe we have to figure it out.

I'll try to find the time to clean up what I was doing and throw it into a branch for people to look at.

I understand the problems you're talking about. However, some of them have simple solutions.

For example you can generate your files as part of the npm prepublish, which is what most libraries I've seen do. The package you pull down looks little like the original repository.

E.g. a TypeScript project might generate definition files and a CommonJS module set. Check out the scripts in this project for example. That's also how a project like RxJS can easily maintain multiple module builds (ES6 modules, globals, etc) from a single codebase. The build can take some time to set up, but once it's done, there's really no maintenance.

As far as the CommonJS thing... The way scripts are consumed today is changing pretty fast. With ES6 modules devs are moving toward tools like Webpack so they can seamlessly deal with multiple module systems. The problem is, this project is written in such a way that it breaks even with WebPack. In order to get it to work as is, you'd have to use import and export loaders to transform the package. At least with CommonJS, it works seamlessly.

If you still want to output a global, is quite simple with a CommonJS codebase and Webpack.

Also, if you'd like some help with this process, I can help write the build for you. I've done a lot of them.

please please merge it!!

@davidlehn sounds like good progress! Thanks for working on this.

Ping. I could use a smoother solution as well. Really want to use this with webpack without having to jump through hoops.

@davidlehn perhaps reproducible builds ( mentioned briefly here: ipfs/distributions#49 ) would help with some of your concerns?

If anyone can check out the source from a given tag, run a reproducible build process, and then end up with the exact same bundle (eg a hash published with the release), then you can check for yourself that the bundle has not been tampered with.

Is there any workaround to get forge working with webpack until this is resolved?

@Gattermeier I'm unhappily using a webpack alias until such a time as I can do this the normal way.

module.exports = {
  // ...
  resolve: {
    extensions: ['', '.js', '.jsx', '.json'],
    root: [PATHS.dev],
    alias: {
      forge: path.resolve(__dirname + '/node_modules/node-forge/js/forge.min.js'),
    }
  },
}

require'ing a pre-built version has one big advantage: i was able to shave off 300kb by vendoring and commenting stuff I don't need (TLS for example) at the price of more complicated updates.

Will this ever be merged or are we forced to use ysangkok's branch forever?

@siebertm I just upgraded to webpack 2 which has tree shaking built in. It automagically cuts out a whole bunch of stuff I wasn't using.

I polished up my similar attempt to solve this: #456
It's a similar approach but I think perhaps the meat of the CommonJS transformation is less intrusive for a first pass. Thoughts?

Please review https://github.com/digitalbazaar/forge/tree/cjs and #456. The branch converts forge to CommonJS, supports webpack and browserify, adds karma testing, and has many other cleanups. I think it addresses everything from this PR. The branch is ready to merge as 0.7.0. If you have feedback, please leave it soon. Thanks.

#456 has been merged as an alternative to this PR.

Is a release going to be made for this?

@joeheyming, Yes. A new 0.7.0 release will include it, but we have to do some additional repository shuffling to deal with bower, etc. before we can release.

It's been two weeks, how about now?

@joeheyming 0.7.0 was released. Almost exactly one year after I opened this PR. Interesting coincidence.

woo! thanks @ysangkok @dlongley and everybody else who contributed. this just stripped a lot of time out of my build process and the team is very happy.

woo! thanks @ysangkok @dlongley and everybody else who contributed. this just stripped a lot of time out of my build process and the team is very happy.

Big thanks to @davidlehn. :)