lasso-js / karma-lasso

Karma plugin for Lasso
http://lasso-js.github.io/karma-lasso/
MIT License
1 stars 11 forks source link

Does not work with lasso 3: plugin lasso-require not found #15

Closed abiyasa closed 6 years ago

abiyasa commented 6 years ago

The new lasso 3, released 3 weeks ago, merged lasso-require into lasso (see CHANGELOG). This breaks karma-lasso, which I accidentally found while making PR to ebay-font (https://github.com/eBay/ebay-font/pull/29).

When I run test that uses karma-lasso, I got the error "plugin module not found for lasso-require". Here's detailed error message from the last build of ebay-font:

04 12 2017 16:14:13.796:DEBUG [framework:lasso]: running command: node node_modules/karma-lasso/lib/helpers/lasso-command.js .test/lasso-config.json .test/lasso.json .test/output.json .test/watch.json 
     Saving optimization output to: ./.test/static
(node:3932) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Error while applying option of "plugin". Cause: Error: Plugin module not found for "lasso-require". Searched from "/home/travis/build/eBay/ebay-font"
    at Object.plugin (/home/travis/build/eBay/ebay-font/node_modules/lasso/dist-compat/config-loader.js:292:43)
    at invokeHandlers (/home/travis/build/eBay/ebay-font/node_modules/property-handlers/lib/index.js:72:29)
    at Object.plugins (/home/travis/build/eBay/ebay-font/node_modules/lasso/dist-compat/config-loader.js:285:21)
    at invokeHandlers (/home/travis/build/eBay/ebay-font/node_modules/property-handlers/lib/index.js:72:29)
    at Object.load (/home/travis/build/eBay/ebay-font/node_modules/lasso/dist-compat/config-loader.js:381:5)
    at create (/home/travis/build/eBay/ebay-font/node_modules/lasso/dist-compat/index.js:89:31)
    at getDefaultLasso (/home/travis/build/eBay/ebay-font/node_modules/lasso/dist-compat/index.js:110:24)
    at Object.<anonymous> (/home/travis/build/eBay/ebay-font/node_modules/lasso/dist-compat/index.js:21:16)
    at next (native)
    at step (/home/travis/build/eBay/ebay-font/node_modules/lasso/dist-compat/index.js:39:191) (config.plugins)
fs.js:642
  return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode);
                 ^
Error: ENOENT: no such file or directory, open '.test/output.json'
    at Error (native)

...

I tried to remove lasso-require from karma plugin but it didn't work; I still got other error, complaining that .test/output.json is not existed.

Do you mind looking into this issue? Thanks a lot!

abiyasa commented 6 years ago

I tried to commented out the code which uses lasso-require (https://github.com/lasso-js/karma-lasso/blob/master/lib/framework.js#L19-L55) but it still fails.

I found out that one of the breaking changes from lasso 3 is that async API method returns promise instead of using callback. So changing this line with callback:

lasso.lassoPage(
    fs.readJsonSync(process.argv[3]),
    function(err, optimizedPage) {
        if (err) {
            console.error(err);
            return process.exit(1);
        }
        var initFile = path.resolve(
            outputDir,
            'file-' + parseInt(Math.random() * 9999999) + '.js'
        );
    ...
);

to promise below, would solve some of the problems

lasso.lassoPage(lassoJson)
    .then(function (optimizedPage) {
        var initFile = path.resolve(
            outputDir,
            'file-' + parseInt(Math.random() * 9999999) + '.js'
        );

        ...

        fs.outputJsonSync(process.argv[4], files);
        process.exit(0);
    })
    .catch(function (err) {
        console.error(err);
        return process.exit(1);
    });

Now, the only problem left is to replace parts which use lasso-require with something, which I don't know how.

Do you have any suggestion, is it safe to just remove that part @pranavjha ?

austinkelleher commented 6 years ago

@abiyasa I went through and updated most of the Lasso plugins to support Lasso 3. Unfortunately, this plugin does not have any tests, so I didn't want to make any assumptions about the code.

You're right about the lassoPage API. That returns a promise only now. In Lasso 2, it returned a promise or took a callback.

For the lasso-require bit, lasso-require was merged into Lasso 3. There is now a configuration option that can be set in the lasso config require. For example: https://github.com/marko-js/markojs-website/blob/master/project.js#L47

So essentially this will need to change https://github.com/lasso-js/karma-lasso/blob/master/lib/framework.js#L19 to actually set the Lasso config require option instead of creating an object with a plugin. Something like (pseudocode):

...

if (!config.lasso.require) {
  config.lasso.require = { transforms: [] }
} else if (!config.lasso.require.transforms) {
  config.lasso.require.transforms = []
}

if (config.autoWatch !== false) {
  config.lasso.require.transforms.unshift('karma-lasso/lib/transforms/watcher');
}

// include the instrumentation transform if the coverage is to be done
if (config.lasso.coverage) {
  config.lasso.require.transforms.unshift('karma-lasso/lib/transforms/coverage');
}

...

Hope that this helps.

abiyasa commented 6 years ago

@austinkelleher Thanks a lot 🙂 I'll try that and will make a PR once it works.

pranavjha commented 6 years ago

@abiyasa sorry didn't see your last comment.. anyways added a PR for the fix. Please let me know if this works. Will wait for your confirmation before publish.

abiyasa commented 6 years ago

@pranavjha No worries! and thanks a lot for the PR. I tested ebay-font to use your master branch and the test passed 👍

Could you remove lasso-require from the peerDependencies? Not a big deal but it's just nice to remove 1 warning from yarn install 🙂

Please publish the new karma-lasso and thanks also for bumping the major version 😊

pranavjha commented 6 years ago

merged https://github.com/lasso-js/karma-lasso/pull/16

abiyasa commented 6 years ago

@pranavjha Thanks a lot for the quick response! Now the builds are fixed