-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
API is confusing #571
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
Comments
I have to agree to with this. The config you provide to the driver function should be the config that object uses. I have looked into changing it, but to be honest I don't have a great deal of experience with modules and it would take me longer than I'd like. Do you have a quick fix in mind @Qrysto ? |
If you create the driver in a function like so:
It will allow you to do the following:
Which is good enough for what I need right now. Closures for the win! |
I had to work around this with a pretty similar approach - always creating the driverObj right before calling |
Super. I can easily fix it when it's been compiled to an IIFE, but my workaround does the trick for now, without needing to fiddle with the source code. |
When I see driver.js's basic usage
I immediately thought that the provided config would be tied to driverObj specifically. But I was misled, and ended up encountering this bug:
Then somewhere in page 1, when I run
driverObj1.drive()
, suddenly the tour configured inconfig2
is run instead ofconfig1
!Turns out that the config is NOT tied to the returned
driverObj
but is always global, and when you calldriver(config)
, it just set thatconfig
globally for ALL the driver objects in existence and created afterward. The returneddriverObj
doesn't play any role here, it's just a set of API that works on the global config and global state. It could just have been exposed from the library without having to call thedriver()
function.So I think the current API is super confusing. I would suggest two options:
If you want to keep the current API - returning a driver object that's initialized with some config - then the provided config should be associated with that returned object no matter how many other driver objects you create afterward.
If you want to make the library a singleton, then
driver()
should be renamed to something likesetConfig()
to reflect what it really does, and it shouldn't return anything. Instead the API functions likedrive()
orhighlight()
should just be exported from the library, because it will use the global config anyway.I can create a PR if this is agreed on.
The text was updated successfully, but these errors were encountered: