computmaxer / karma-jspm

Other
74 stars 52 forks source link

Add es6-promise polyfill if system-polyfills doesn't exist. #187

Closed dwilson6 closed 7 years ago

dwilson6 commented 7 years ago

This is the case in jspm 0.17 beta >33 because systemjs no longer includes system-polyfills.

dwilson6 commented 7 years ago

This was working for me testing locally but now that I switched my test project to point at my branch its not working so hold off on merging this.

dwilson6 commented 7 years ago

This can be merged now

SerkanSipahi commented 7 years ago

@dwilson6 thank you for your pull-request but i think you can resolve it very easily, without merging, by providing over karma.conf.js (loadFiles), see example config :)

jspm: {
    // ...
    // ...
    loadFiles: [
        // includes regenerator-runtime: useful for async await
        'jspm_packages/npm/babel-polyfill*/dist/polyfill.js'
    ],
    serveFiles: [
        // ...
        // ...
    ]
}
dwilson6 commented 7 years ago

Unfortunately, that won't work because the latest version of system.src.js (>0.20) has some global references to Promise so a promise polyfill needs to be loaded before system.src.js.

SerkanSipahi commented 7 years ago

@dwilson6 hmm, you are right ! let me do some researches.

ffflabs commented 7 years ago

:+1: I just forked this repo wanting to do the same (using Bluebird, tho). It's nice to see someone already made it!

ffflabs commented 7 years ago

@SerkanSipahi please merge this pe and give us a new tag release.

SerkanSipahi commented 7 years ago

@amenadiel @dwilson6 Sorry guys for the late response. We have to do it differently (merge). Its not clean if we merge babel-polyfill as hard dependency to jspm and furthermore bluebird is also a hard dependency and its very slow when using with many promises. As i said, i will check for another solution. Currently i have only time on the weekend.

SerkanSipahi commented 7 years ago

@amenadiel @dwilson6 What do you think about that? You as developer can decide which files (babel-polyfill or bluebird, etc.) should be loaded before loadFiles and serveFiles.

Its just a suggestion!

jspm: {
    beforeFiles: [
        'node_modules/bluebird/bluebird.js',
                // or
                // 'node_modules/babel/babel-polyfill.js'
        ],
    loadFiles: [
        // ...
    ],
    serveFiles: [
        // ...
    ]
}
ffflabs commented 7 years ago

That sound reasonable too.

However, you'd still need to add part of the logic provided by @dwilson6 to detect if system-polyfills.src.js is present on the system. Otherwise, you'll try to load it and the runner will crash.

SerkanSipahi commented 7 years ago

@amenadiel @dwilson6

However, you'd still need to add part of the logic provided by @dwilson6 to detect if system-polyfills.src.js is present on the system. Otherwise, you'll try to load it and the runner will crash.

not really :) We can document something like this in README.md => if you use jspm 0.17 beta >33 and need e.g. system-polyfills, please add your file/s in beforeFiles

dwilson6 commented 7 years ago

My thoughts on your approach. karma-jspm in essence wouldn't work with the latest jspm 0.17 beta without someone doing extra work which isn't great but I see your point about not always loading a promise polyfill.

Phantomjs 2.5 which is in beta adds native support for promises so your solution is a stopgap which makes it better. It might be nice to print a warning message if system-polyfills is not found and the browser is phantomjs in the meantime (unless there's a way to actually detect if the browser supports promises).

SerkanSipahi commented 7 years ago

@dwilson6 sounds good :)

ffflabs commented 7 years ago

@SerkanSipahi @dwilson6

not really :) We can document something like this in README.md => if you use jspm 0.17 beta >33 and need e.g. system-polyfills, please add your file/s in beforeFiles

so you would make the script try to load system-polyfills.src.js anyway, because worst-case-scenario is seeing a warning in karma if it isn't found.

That wouldn't get in the way of CI so it works for me.

dwilson6 commented 7 years ago

@SerkanSipahi should I implement your proposed solution or are you already working on it? Just making sure I'm not duplicating effort.

SerkanSipahi commented 7 years ago

@dwilson6 sure, thanks :)

dwilson6 commented 7 years ago

I updated this PR with the new implementation.

SerkanSipahi commented 7 years ago

@dwilson6 thank you for the PR. Its merged, tagged (2.2.3) and published also in npm :)

ffflabs commented 7 years ago

It works!

However, the use case provided in README.md assumes that Phantom has access to node_modules which in my case wasn't true. It should be noted that this polyfill should be inside whatever folder karma considers as its base.