Broken when using JS aggregation: DOMException: IntersectionObserver constructor: rootMargin must be specified in pixels or percent.

Okay I think I found the reason! The library definitin is wrong, it must point out that the code is already minified!
See https://www.drupal.org/docs/develop/creating-modules/adding-assets-css-j...

We could also consider renaming tour.js to tour.min.js or for clarification, but of course that's totally optional.

While I'm 100% sure the current implementation is wrong and the fix is right, we're still seeing the same issue even after the fix.

We should nevertheless fix this and see if the issue can be reproduced before and after (manually). @smustgrave could you maybe give it a try?

Thanks @smustgrave. Code-wise the fix is clearly correct, I'd say... so I guess we're running into this for further reasons. Some other modules like blazy also use IntersectionObserver and maybe conflict. Still the error clearly appears in Tour module as described with aggregation enabled. For that reason, I'll set this to NW again, after merge.

Besides fixing our possible conflict, it would be great to run the tests with and without aggregation enabled!

With the fix merged and the feedback from @smustrage, I'm setting the priority to normal here.

Okay tests are also green with aggregation enabled (if it works as expected in the tests).

Still, we might add a "WithAggregation" subclass of each test for the future using this setting? So tests should run with and without aggregation? Or does anyone know a better approach.

And I'll try to find the root cause for us.

FYI: The issue is still present with Shepherd.js 13.0.3 (Tour 2.0.13)

Debugging this I found out that the variables in our case are:

{ rootMargin: "-573px NaNpx NaNpx -15px", threshold: 1 }

Final results so far:
i.clientWidth
and
i.clientHeight
are undefined with aggregation enabled and const i = $(e); (jQuery) behaves differently.

So maybe a "wrong" jQuery is used with aggregation or the asset order is wrong with aggregation or something like that... Crazy!

In #3540255: Race condition in _removeIrrelevantTourItems() e.g. with BigPipe if page is not yet fully loaded @smustgrave wrote:

Maybe the weight of the library needs to be altered? Some changes introduced in 11.1 broke tour because of the order assets were loaded

@smustgrave or do you think this might be caused by the /core/themes/stable9/js/tour.js overrides we're seeing?

Thanks, we'll dig deeper... Might also be a conflict with another module. Back to #14 for us.

Thanks @smustgrave sadly the reordering doesn't fix it. But we just found out that preprocess: false fixes the issue. As long as dist/tour.js is not part of an aggregated js file, it's fine!

Possible causes:
I could imagine that this is a combination of different things:
1. "weight" property: I don't know why weight: -1 is used, but it would be great to tell why (and add a comment for the future). If it's not needed, it should be removed. Maybe it just runs to early as part of the Drupal aggregate?
2. The build/tour.js compiled source pollutes the global scope with a lot of functions. Aggregated with other JS in the same JS file, that may conflict if any other module minifies to the same variable names!
The code looks like this:

/*! shepherd.js 13.0.3 */ function tn(e) {
    return e instanceof Element;
}
function ke(e) {
    return e instanceof HTMLElement;
}
function Z(e) {
    return typeof e == "function";
}
function Ce(e) {
    return typeof e == "string";
}
function T(e) {
    return e === void 0;
}
[...]

Possible solutions:
1. Use the preprocess: false quickfix from the MR. I'd even vote to merge it, until we clearly know how this happens and how to fix it. The impact should be minimal, but it removes the side-effects. Maybe what we see is even a side-effect of #3416508: Skip generating aggregates that won't be used for stale asset requests but I don't think so.

2. Do not pre-built the sources and do not aggregate tour.js with the library in dist. Imho that is not Drupal best-practice and may lead to such side-effects. Instead use the libraries functionality from Drupal for local sources or CDN: https://docs.shepherdjs.dev/guides/install - if the maintainer agrees, let's do that in a separate issue. There are further benefits, for example the user can independently upgrade to a (paid if commercial) Shepherd v14. As long as the modules tour.js is bundled, that seems impossible. Furthermore we won't need the untypical build step anymore.
Instead asset-packagist or a simple download into libraries folder or CDN should be used, like in most other contrib modules.

Short-term I'd vote for (1) and long-term for (2)! :)

Fix attached as static patch until this is finally solved :)

Thanks @smustgrave what do you think, should we merge the workaround for the meantime and open a separate issue to check the library separation? Before that we should definitely check the reasons it was done this way in #3480031: Update shepherd.js library!

can we remove the todo and I'll merge.

Mhm but shouldn't we better keep it, as it shouldn't be there forever, due to its workaround nature?

And unless it becomes a major problem not sure we need to break up how it’s compiled. I’m 90% positive moving to it’s own library and loading like it did in core no longer works with v13 of shepherd. Core was using v10

Should we give it a try in a separate issue and see if it works? Shouldn't be too hard, I guess?

And the weight was added because in 11.1 asset ordering was changed and tour completely stopped working because it was loading last

MIGHT be that this could also be resolved in the separation issue, if the reason is the library bundling?

In #3480031: Update shepherd.js library @darvanen wrote:

It is possible to include the shepherd library without using a bundler but ensure that it loads before the tour code is not something I am familiar with

When using it from CDN or library, having the library as dependency of the tour initialization should happen like for any other JS?

Thanks, I removed the @todo!

Regarding BigPipe: Yes and the other part would be a test with and without aggregation. With the fix it should be green, without not... Do you have any ideas regarding that? (#9 )

Still I'm fine to leave that out, at least for now with the fix, just saying because BigPipe addition is similar...

Nice @smustgrave! Maybe add a comment why we're doing that? I don't think it's self-explaining.
I'm fine with that, while it probably won't hit 100% ...