johnsoftek / plugin-jade

Jade loader plugin for jspm
MIT License
13 stars 5 forks source link

Separate runtime dependency #1

Closed guybedford closed 9 years ago

guybedford commented 9 years ago

It could be good to load the runtime with:

  var runtime = "var jade = require(" + module.id + "/jade-runtime);\n\n";

This way, you're not assuming the plugin name (jade), and are inserting it dynamically.

Also, having the runtime as a separate require means that the full compiler won't be built into the bundle for production.

johnsoftek commented 9 years ago

@guybedford As it stands, the compiler would only be included in development.

Regardless, I have simplified the require of the Jade runtime. The round-about logic was used at some point to overcome the disambiguation problem.

The solution, at present, is to include a copy of jade.js (as jade-compiler.js)and runtime.js (as jade-runtime.js) from the jadejs github repo. Both files are also present in the npm repository but there is a slight difference which makes it difficult to use with jspm.

The npm versions of jade.js and runtime.js have a comment line at the beginning of the file:

"format cjs";

The jade.js file is packed with required files a la browserify.

So, if you require jade.js, all jade requires should be handled internally. But systemjs behaves differently in the presence or absence of the "format cjs"; line. When the line is present, all requires are resolved external to the file. When it is omitted, all requires are resolved from the files packed within the file.

Which approach do you prefer? Copying files from the github repo? Or linking to the npm repo files and working around the require problem?

johnsoftek commented 9 years ago

The Jade build process creates jade.js and runtime.js using browserify. I assume that browserify inserted the "format cjs"; line in both files.

I tried changing the "format cjs"; line to read "format amd";. That worked.

I also tried System.meta['jade'] = {format: 'amd'} and that worked too.

So is browserify wrong? Is systemjs wrong?

guybedford commented 9 years ago

@johnsoftek the "format cjs" line is inserted by jspm because all modules on npm are assumed to be CommonJS. To override this, add a package override (jspm install npm:jade -o "{ format: 'amd' }"). I'm happy to merge this into the jspm registry if it works.

The suggestion above will allow this plugin to build with SystemJS builder into a bundle, and building in the runtime as well only when needed.

johnsoftek commented 9 years ago

@guybedford. Oh, jspm inserts the "format cjs" line!

I think it is cleaner for the plugin-jade github repo to contain the browserified versions of jade.js (224KB) and runtime.js (6KB) then to add npm:jade@1.9.0 (18MB) as a dependency in package.json. Do you agree?

guybedford commented 9 years ago

@johnsoftek perhaps use github:jadejs/jade instead then. That is just 1MB, and means it becomes a proper package-shared dependency.

If that works for you, we can add this to the registry as the default install of jspm install jade-compiler say.

johnsoftek commented 9 years ago

@guybedford. Sounds good. I will work on it.

johnsoftek commented 9 years ago

I agree jade-compiler = github:jadejs/jade should be added to the registry. Want a PR?

I had fun requiring runtime.js from the same repo. Take a look at https://github.com/johnsoftek/plugin-jade/blob/github-jade/jade.js. If you have any suggestions, I'm all ears.

guybedford commented 9 years ago

Sure adding it to the registry can work.

Note that in https://github.com/johnsoftek/plugin-jade/blob/github-jade/jade.js#L7, you can't guarantee that jade is a defined dependency as dependencies are scoped to the package they are loaded in only.

That's why I suggested doing something contextual. Not sure what would be best there though.

johnsoftek commented 9 years ago

Hmm. From your earlier suggestion, module.id is github:johnsoftek/plugin-jade@github-jade/jade and results in:

var jade = require('github:johnsoftek/plugin-jade@github-jade/jade/runtime');

and that does not work.

If I use "var runtime = "var jade = requir" + "e('" + module.id + "').runtime;\n\n";, it works. I have to obfuscate the require to avoid an error in the dependency tree scan.

I am starting to think that importing runtime.js is too messy. Maybe it would be best to keep the master version that contains copies of jade.js and runtime.js.

guybedford commented 9 years ago

I think you would need to do:

System.normalize('jade-runtime', { name: module.id }).then(function(normalized) {
});

To do a normalization of the jade-runtime module from within the current module.

johnsoftek commented 9 years ago

@guybedford I have reverted to using Jade from npm. However, I found it too difficult to require both jade and runtime from npm:jade. So, for now, I am not including jade-runtime in plugin-jade.

To use the plugin:

jspm install jade
jspm install npm:jade-runtime