rolaveric / karma-systemjs

Karma plugin for using SystemJS as a module loader
MIT License
40 stars 19 forks source link

Support a imports config property for systemjs config #76

Open guybedford opened 8 years ago

guybedford commented 8 years ago

Just been playing around with this, and it seems great. I was just thinking if I want to write my tests themselves as modules loaded through SystemJS, then it can be useful to have configuration to set the tests up as imports themselves, something like:

systemjs: {
  import: ['test/main.js']
}

Where it then automatically imports those files in the browser through SystemJS.

guybedford commented 8 years ago

Ahh, I see that the files field is in fact loaded with SystemJS.import.

This seems like a non-standard thing to do with Karma though as it is changing the meaning of the files property?

Perhaps it should still be a custom systemjs property here.

rolaveric commented 8 years ago

You're right, it is very custom behavior for Karma. But it felt like the least-surprising approach for users. All the standard Karma configuration tells you to put your file globs into files. If those files need to be transpiled, you're just going to get a lot of errors. With the current approach, running all include: true pattern through System.import, the worst you get is a bit more processing than is absolutely required.

Still, you make a good point that there's no option for powerusers who fully understand how SystemJS works to System.import files without any magic to confuse the issue. There's already a serveFiles and an includeFiles property - how about an importFiles property to match the standard, and I can rewrite the README to encourage their use. That'll give us the best of both worlds: New users working off default Karma configuration won't be too confused, and powerusers can specify exactly what they want.

Thoughts?

guybedford commented 8 years ago

If you look at the RequireJS instructions (https://karma-runner.github.io/0.13/plus/requirejs.html) they advocate using the included: false option in files. I do think it would make sense to follow the precedent set there.

I've been working a simplified bare-bones fork implementation here as I'm trying to include full code coverage with the SystemJS translate hook and preprocessing workflows (running transpilation on the server), and it turned out necessary to support some of this.

It's very much experimental, but I can share the progress I make and then perhaps we can work out if merging might be suitable? I'm really appreciative of your ongoing work on this project, so it would be great to work towards some synthesis of ideas here.

rolaveric commented 8 years ago

Sounds great. I'm more than happy to talk through ideas, look through your progress, and merge in changes once they're ready to go. Whatever gives us a better plugin.

guybedford commented 8 years ago

@rolaveric I've got something just about working at https://github.com/guybedford/karma-systemjs-experiment, exploring some ideas which would complement what is happening in this repo. I'm aiming to do a writeup on the principles and approach tomorrow, which I can share with you privately first if you like as well? Just let me know how I can reach you for that.

Then I'm not really sure what the best way would be to go about a merging here, so perhaps let me know what you think might work? I can potentially try and form it into a PR for this project, or we can break off features from there? I'd be interested to hear your feedback here, and thanks so much for being open to these considerations.

rolaveric commented 8 years ago

Looks great. I'll test it out when I get home from work tonight (6:30am right now in Sydney) and give you some user feedback. If you want to send me anything privately you can email it to "rolaveric at gmail dot com".

It's significantly different from this repo, so we're talking about less of a 'merge' and more of a full blown replacement - which I'm all for. I'd be tempted to just transfer the npm name to you, but then the issues (open and closed) from this repo get orphaned. Or maybe that's not a big deal?

guybedford commented 8 years ago

Yes it's a different approach, but there could certainly be splicing done between the two. I guess it could be a major release if you wanted to do a replacement, but it would be good to know from your feedback if the experiment can work in the use cases and scenarios you have been dealing with, as I don't have near the level of battle testing knowledge this project has faced.

rolaveric commented 8 years ago

I had a go with using your plugin against a repo I'm currently playing with: https://github.com/rolaveric/ng1-to-ng2-example/tree/karma-systemjs-experiment I couldn't include the repo as an npm dependency (Something missing from the package.json perhaps?) so I cloned it and used npm link.

First issue I had was from a "*": "node_modules/* property I had creating some strange module URLs. Once I resolved that I got the following:

Unhandled rejection TypeError: Cannot convert undefined or null to object
    at Function.keys (native)
    at remapLoadRecord (C:\Users\rolaveric\WebstormProjects\karma-systemjs-experiment\node_modules\systemjs-builder\lib\compile.js:82:12)
    at compileLoad (C:\Users\rolaveric\WebstormProjects\karma-systemjs-experiment\node_modules\systemjs-builder\lib\compile.js:88:20)
    at C:\Users\rolaveric\WebstormProjects\karma-systemjs-experiment\node_modules\systemjs-builder\lib\builder.js:497:12
    at tryCatcher (C:\Users\rolaveric\WebstormProjects\karma-systemjs-experiment\node_modules\bluebird\js\release\util.js:16:23)
...

I drilled down a bit and found that load was actually false. I'll take another look at it tomorrow.