driver.js icon indicating copy to clipboard operation
driver.js copied to clipboard

API is confusing

Open Qrysto opened this issue 11 months ago • 4 comments

When I see driver.js's basic usage

const driverObj = driver(config);
driverObj.drive();

I immediately thought that the provided config would be tied to driverObj specifically. But I was misled, and ended up encountering this bug:

// Initialize driver.js in page 1
const driverObj1 = driver(config1);

// Initialize driver.js In page 2
const driverObj2 = driver(config2);

Then somewhere in page 1, when I run driverObj1.drive(), suddenly the tour configured in config2 is run instead of config1!

Turns out that the config is NOT tied to the returned driverObj but is always global, and when you call driver(config), it just set that config globally for ALL the driver objects in existence and created afterward. The returned driverObj 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 the driver() 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 like setConfig() to reflect what it really does, and it shouldn't return anything. Instead the API functions like drive() or highlight() 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.

Qrysto avatar May 17 '25 12:05 Qrysto

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 ?

Murphybro2 avatar May 23 '25 12:05 Murphybro2

If you create the driver in a function like so:

const driverObj = window.driver.js.driver();

function createDriver(config) {
	return {
		Start: function () {
			driverObj.setConfig(config);
			driverObj.drive();
		},
		Next: function () {
			driverObj.moveNext();
		}
	};
}

It will allow you to do the following:

// Initialize driver.js in page 1
const driverObj1 = createDriver(config1);

// Initialize driver.js In page 2
const driverObj2 = createDriver(config2);

Which is good enough for what I need right now. Closures for the win!

Murphybro2 avatar May 27 '25 12:05 Murphybro2

I had to work around this with a pretty similar approach - always creating the driverObj right before calling drive then just discard it after every use, instead of creating driverObj once and keep the reference for reusing. It worked for me, but of course I would be more happy if the original API is improved.

Qrysto avatar May 28 '25 10:05 Qrysto

I had to work around this with a pretty similar approach - always creating the driverObj right before calling drive then just discard it after every use, instead of creating driverObj once and keep the reference for reusing. It worked for me, but of course I would be more happy if the original API is improved.

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.

Murphybro2 avatar May 28 '25 10:05 Murphybro2