🇩🇰Denmark @googletorp

I have done a couple of things now, to summarize what we have.

1. Improved existing test cases in AssetResolverTest and correct a wrong assertion (different libraries, same timestamps should have 2 / 4, not 2/3). This was a result of the way libraryDiscovery was mocked
2. Create a failing unit test case where a CSS on library influence the contents of JS groups
3. Adjusted fix to ensure that new test case is now green (needed to both do sorting and re-calculation of dependencies from minimal dependencies to get a working solution

With regards to the comment in the PR, we need to double unset (explained in detail in the PR). The root issue is caused by CSS dependency changing the order or dependencies which can result in different groups. The reason why this fails is that when Drupal generates the aggregated it does so from the minimal JS only libraries.

The test cases for the fix is now all green (we got lucky with the ckeditor test case which was failing before), so I think this is ready now.

I spent quite some time trying to figure out what is going on. I also saw the issue with gin theme and had some node forms which didn't work depending on which additional libraries were pulled in via the field_group module.

Originally I thought the problem was the ordering of libraries, but upon further investigation it seems the root issue is that the order of all libraries vs the ordering of only js assets isn't necessarily the same, which then produces the error @swentel mentions in #11.

I closed the MR from #4 since it doesn't necessarily fix things and made a new patch in !9219 which I think actually solves the issue.

I don't know how to do a an actual test case for this - I made a test module which has two pages, one failing and one before the fix where both are working after the fix. For some reason some functional JS unit tests are failing in the MR which introduces the test module used to demonstrate the issue, which seems really strange to me, as the introduction of a test module shouldn't make test fails, so wonder if something else is going on.

I'll try to see if I can come up with a way of testing this, but would be great if some one could take a look at things mentioned and provide feedback on the suggested fix.

I did a simple approach which is to ensure that the libraries that needs to be processed is sorted. This means that the order of the libraries doesn't influence the specific groups that are created, which in turn ensures as consistent behaviour.

I'm not sure if and how we should write tests for this, as the whole cycle is pretty complicated and not sure how we could write tests to actually capture this.

googletorp → created an issue.

DieterHolvoet → credited googletorp → .

Create a MR with a fix.

googletorp → created an issue.

I've used this in the past and compared to the alternative (which is to fetch everything in one go) this is a major improvement. This is also an opt-in feature, so while making a lot of requests can cause problems, it's something the developers using this can determine if it's a problem or not. From what I understand if you're not using the feature we won't have any performance change.

Maybe it's better to get it in, solve a lot of people's problems and get real life feedback on which performance issues would make sense to address.

I think this is RTBC.

I wont go deep into this issue, but maybe worth for me to quickly touch on this.

I built the original version of the module (because I needed it) and did maintain it for about ~5 years with Centarro slowly taking over and owning the D8+ release fully.

In the many years I was active part of the drupal commerce community and worked on many drupal commerce projects I found that the main thing I was wishing for - was more modules to handle difference use cases like

- shipping
- discounts
- coupon codes
- admin reports

or maybe the module existing, but wasn't fully polished and/or difficult to use.

If adding advertisements can help fund modules which can strengthen the drupal commerce ecosystem, what's not to like? I get that people don't enjoy looking at ads in general, but building and maintaining modules is a tremendous amount of work. Unless you have tried it yourself, it's hard to understand how much work it requires. We need to look at ways of doing that in a sustainable way instead of fighting such initiatives.

Ryan didn't involve me in this decision but I fully support this direction and seeking partnerships which can help pay for maintenance of modules like this.

This has been fixed in 2.1.1

The patch at github seems to have a lot of issues.

Trying to execute the button I get: Uncaught CKEditorError: t.closest(...).querySelector(...) is null

When using same editor outside paragraphs you also get a similar error on load about querySelector which crashes editors if you have multiple on the same page. In general seems like checks are missing for the different selectors. Example

document.querySelector('XX').something

will cause JavaScript TypeError if XX is not present on the page, so you always need to check the output of querySelector and act accordingly if what you expected is not on the page.

I have created a test case which shows the failing issue and created a fix for the problem in the issue fork.

In general the problem is that before we did this:

foreach ($libraries as $library) {
  $with_deps = $this->getLibrariesWithDependencies([$library]);
  // We don't need library itself listed in the dependencies.
  $all_dependencies += array_diff($with_deps, [$library]);
}

The key problem is here:

$all_dependencies += array_diff($with_deps, [$library]);

I guess however wrote this assumes that this gives you the sum of the two arrays, but that is not the case, this only add the dependencies that exceeds the amount of dependencies we have of the left have side. The test case illustrates this.

googletorp → created an issue.

Rerolled the patch from #8 by manual comparing what was required.

googletorp → made their first commit to this issue’s fork.