Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How can i access Chromy.addCustomDevice? #608

Closed
deap82 opened this issue Dec 1, 2017 · 13 comments
Closed

How can i access Chromy.addCustomDevice? #608

deap82 opened this issue Dec 1, 2017 · 13 comments

Comments

@deap82
Copy link
Contributor

deap82 commented Dec 1, 2017

How can I access the static Chromy.addCustomDevice function?
I tried to do

const Chromy = require('chromy');

in my onReady.js, but just importing it like that causes the "backstop reference" command to fail.

Any leads appreciated.

@garris
Copy link
Owner

garris commented Dec 1, 2017

Please take a look at the example scripts. You don’t need to “require” Chromy. It’s passed as an argument to the callback closure which your script runs in.

@deap82
Copy link
Contributor Author

deap82 commented Dec 4, 2017

@garris I did have a look at the example scripts but can't find an example for this. This specific method - addCustomDevice - is not available on the chromy instance, it's static on the type.

If I want to use

chromy.emulate('myCustomDeviceConfig');

I need to first call

Chromy.addCustomDevice({ name: 'myCustomDeviceConfig', /.../ });

Notice chromy vs Chromy.

If I do
console.log('chromy.emulate', chromy.emulate);
I can see the body of the emulate function logged out to console.

If I do
console.log('chromy.addCustomDevice', chromy.addCustomDevice);
I get an exception.

@garris
Copy link
Owner

garris commented Dec 4, 2017

Oh, didn’t notice that. If it’s a static method you can just import it. But you might need to alias it to avoid collisions. Does that work?

@deap82
Copy link
Contributor Author

deap82 commented Dec 5, 2017

I created a small gulp task and when I run this I get the function logged out:

gulp.task('backstop-foo', function (cb) {
    var ChromyStatic = require('chromy');
    console.log('ChromyStatic.addCustomDevice', ChromyStatic.addCustomDevice);
    cb();
});

But when I try this in my onReady.js for backstop:

module.exports = function (chromy, scenario, vp) {
    var ChromyStatic = require('chromy');
    console.log('ChromyStatic', ChromyStatic);
};

I get an error when I run "backstop reference", this is the output I get in console:

BackstopJS v3.0.32
Loading config: C:\[SOME-PATH]\backstop-settings.js
COMMAND | Executing core for reference
createBitmaps | Selected 1 of 1 scenarios.
Starting Chromy: {"chromeFlags":["--disable-gpu","--force-device-scale-factor=1","--disable-infobars=true","--window-size=375,667"],"port":9222,"waitTimeout":30000,"visible":false}
CREATING NEW REFERENCE FILES
COMMAND | Command reference ended with an error after [0.572s]

I'm running "backstop reference" with the --configPath flag, so I tried to access it from that .js file like this:

var ChromyStatic = require('chromy');
console.log('ChromyStatic.addCustomDevice', ChromyStatic.addCustomDevice);

When I run that I get this log:

BackstopJS v3.0.32
Loading config: C:\[SOME-PATH]\backstop-settings.js
Error NestedError: Could not require module 'chromy'

@deap82
Copy link
Contributor Author

deap82 commented Dec 7, 2017

Hello again @garris,
To solve my problem for the moment I made a fork: https://github.com/deap82/BackstopJS

It makes it possible to define my viewports like this;

        "viewports": [
            {
                "label": "phone",
                "width": 375,
                "height": 667,
		"emulate": {
                    "name": "phone",
                    "width": 375,
                    "height": 667,
                    "deviceScaleFactor": 2.0,
                    "pageScaleFactor": 1.0,
                    "mobile": true,
                    "userAgent": "Mozilla/5.0 (iPhone; CPU iPhone OS 9_1 like Mac OS X) AppleWebKit/601.1.46 (KHTML, like Gecko) Version/9.0 Mobile/13B143 Safari/601.1"
                }
            },
            {
                "label": "desktop",
                "width": 1920,
                "height": 1080
            }
        ]

There are two parts of my problem, the commit I've done to my fork (deap82@0205f7d) solves the problem of defining my own custom devices for emulation.

The other problem is that when I used the "iPhone6" emulation device included in Chromy (defined here: https://github.com/OnetapInc/chromy/blob/master/src/devices.js), the width/height is configured as twice as what it actually should be. Running backstopjs with that emulation gives me screenshots of the tablet design of my application. I suppose it has to do with Retina and it seems to me that the "deviceScaleFactor" of the emulate object isn't taken into account when chromy and/or backstop is doing its' work.

Also, I tried to get rid of specifying the width/height both in the viewport and the emulate object above, but when they're not in the viewport object backstop fails.

If you have any pointers about these things and what I've done in my fork, I'd be happy to rework this into an acceptable PR.

@kiran-redhat
Copy link
Contributor

kiran-redhat commented Dec 7, 2017 via email

@deap82
Copy link
Contributor Author

deap82 commented Dec 7, 2017

Exactly @kiran-redhat, that's what I've enabled in my fork of backstop as I couldn't find any other way to access Chromy.addCustomDevice.

@garris
Copy link
Owner

garris commented Dec 7, 2017

@deap82 would this have worked if you attempted to implement inside onBefore as opposed to onReady? I think by the time onReady is running it would be too late.

@garris
Copy link
Owner

garris commented Dec 7, 2017

@kiran-redhat correct, backstop does not currently support this.

@deap82
Copy link
Contributor Author

deap82 commented Dec 9, 2017

@garris

When I try this in onBefore:

var ChromyStatic = require('chromy');
console.log('ChromyStatic', ChromyStatic);
console.log('ChromyStatic.addCustomDevice', ChromyStatic.addCustomDevice); 

I now get an error output that might explain the problem:

COMMAND | Command reference ended with an error after [0.568s]
COMMAND | Error: only one instance of babel-polyfill is allowed

(To get it I uncommented " // logger.error(error);" in backstop core/command/index.js. I would suggest this log to always be output, as it gives a lot more meaningful error messages when something goes wrong and it could even be about something the user did in his onBefore/onReady scripts.)

I'm not sure but also it seems to me that even if it worked requiring in chromy myself, it would lead my code to go against a different installation of chromy then the one that backstop uses;

node_modules
   |- backstopjs
   |   |- node_modules
   |       |- chromy (*1)
   |- chromy (*2)

*1 backstop uses this chromy and I use this chromy when calling .emulate(/.../) in onReady.js.
*2 My code in onBefore.js tries to use this chromy.

But again, I say this with reservation as I'm not sure at all how Node handles this kind of situation. But it seems to me the babel-polyfill package includes a warning about this kind of scenario, and chromy does not. Maybe this is something that could be solved with "npm link" but if that's the case, wouldn't it be easier/better if the backstop configuration either had some kind of support for this through the config object OR at least expand parameters sent to onBefore/onReady to also include the static Chromy reference? To me it makes a lot of sense to associate the emulation info with the viewport definitions, but I can of course understand if you don't want backstop to have too many dependencies on the internals of Chromy.

What's your thoughts about my other issue with using the chromy built in emulation for iPhone6?

@deap82
Copy link
Contributor Author

deap82 commented Dec 21, 2017

Hi @garris, any new thoughts on this? I'm open to any approach that enables use of addCustomDevice (and benefit from future updates) and could put some time into a PR over the holidays. Just need some input from you as I feel you might not agree with the above approach.

@garris
Copy link
Owner

garris commented Dec 21, 2017

Hi @deap82 sorry for the delayed responses. I am actually on vacation now — but I wanted to at least give you a reply because I appreciate the thought and effort you are putting into improving backstop for yourself and likely others.

I don’t have time to review backstop internals at the moment — but at a high level I can probably give some comments to unblock your effort.

Maybe the best option is to simply use the same Chromy singleton that is internal to backstop and make this accessible as an argument in both the onBefore and onReady scripts.

This way as features are needed, users have complete access to the all object props.

Would this enable you to move forward?

Also, as a matter of the api— I think it’s a bit cleaner to flatten your viewports example — to not add that emulate property and instead extend the viewports property that already exists there. Then in the onBefore script you can write your own emulation property handler with the Chromy class object methods you added to the onBefore argument list.

How does that sound — is that a sensible approach and does it get you to a useful place?

@deap82
Copy link
Contributor Author

deap82 commented Dec 22, 2017

@garris
I've made a pull request now; #621

Happy Holidays! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants