search

titusfortner / watir_angular

Additional support for using Watir with Angular based websites
MIT License
11 stars 3 forks source link

use inheritance instead of monkey patching #2

Closed akostadinov closed 6 years ago

akostadinov commented 7 years ago

This is a very WIP PR to fix #1. Just to check whether this direction would be acceptable. I'll update spec and documentation once I have a green light.

It uses inheritance instead of monkey patching the base Watir class. It would allow users to instantiate WatirAngular:Browser instead of affecting all usage of Watir::Browser.

It relies on watir/watir#666 for injecting the locators. Idea is to create your Angular browser but don't affect other Watir usage.

P.S. I commented out constant settings because it seems watir falls back to default locators when namespace is missing such. I haven't done too much testing but seems to work for my simple queries.

akostadinov commented 6 years ago

bump, @titusfortner , could you review where I'm going with this pull request before I spend time on changing unit tests and documentation. I'd like to understand if the changes would be acceptable before spending more time.

titusfortner commented 6 years ago

Yeah, I'd like people to get the benefits of using this by only having to require the gem rather than changing the code. I agree that the monkey patching is bad, but it'll get addressed when we implement the Watir Executor in Watir 7.

And yes, I never removed the constants after I added support in Watir 6.8.3 to make it work without it. I'll get that working now.

Thanks.

akostadinov commented 6 years ago

@titusfortner , will this Executor in watir 7 allow selectively enable/disable the functionality? I find monkey patching evil mostly because it performs changes for all use cases, even those where these are not beneficial or harmful. If Executor still does a global change, then this would be pretty much equivalent.

Same with any global state btw. Global state of any kind may simplify the trivial use cases but makes some use cases impossible. I see general tendency in gems to use a lot of global state and switch to instance state over time. I'm referring for example to rest-client gem.

titusfortner commented 6 years ago

Watir Executor will allow you to insert code to execute before or after various methods in Watir. So you won't need to monkey patch, just tell Watir what code to execute after browser initialization. Yes, the code would be equivalent, just no monkey patching.

Let me see what makes sense when I finish refactoring this code and you can tell me what you think. Thanks for your feedback on this.

akostadinov commented 6 years ago

Important for me is not so much implementation details. It is global state that is highly undesirable in my opinion. If one can choose to add additional hooks per browser instance, then this would be very useful. If it is implemented as a global state, I'd probably opt out and add the code to the test framework - to be executed only for the desired browsers.

It is not at all caused by Watir, but for me all this browser testing is a huge time sink. All around minor behavior differences between browsers, versions, (non)legacy drivers, etc. Basically everything may need tweaking thus doing things globally is a potential trouble down the road.