Script type="module" issue

[Skrypt]

Describe the bug

When using <script asp-name="something" type="module"> the dependent scripts also gets a type="module" parameter added to its script tag if it is defined in a ResourceManagementOptionsConfiguration as a dependency.

To Reproduce

            _manifest
                .DefineScript("media")
                .SetUrl("~/OrchardCore.Media/Scripts/media2.js")
                .SetDependencies("vuejs:3", "sortable", "vuedraggable:3")
                .SetVersion("2.0.0");

<script type="module" asp-name="media" version="2" at="Foot"></script>

Look at the script added and all of the dependent scripts will have a type="module" added to them.

Expected behavior

Only the main script should have type="module" added.

[sarahelsaig]

I'm not sure what causes the problem, but I know how to avoid it. If you declare the type attribute in the resource definition instead of the tag helper, then it will work correctly. Also I believe this is a safer and more correct approach anyway, because if you write a module script then you will always need the type="module". So expecting it to be marked as such everywhere it's used would introduce a new point of failure. With your example this should work:

            _manifest
                .DefineScript("media")
                .SetUrl("~/OrchardCore.Media/Scripts/media2.js")
                .SetDependencies("vuejs:3", "sortable", "vuedraggable:3")
                .SetVersion("2.0.0")
                .SetAttribute("type", "module");

<script asp-name="media" version="2" at="Foot"></script>

Here is a simple example I knocked together to verify the above:

(as you can see the parent-script.js's <script> element doesn't get infected with the type="module" attribute)

[Skrypt]

Issue is that it will copy all attributes set on the script tag to the child elements. But that is "by design" which I'm not sure it should.

[sarahelsaig]

For sure, I just wanted to point out that it shouldn't block what you are working on.

I agree, inheriting attributes doesn't seem like a great design choice when expanding dependencies. The offending bit of code is here. Looking over what RequireSettings.Combine(RequireSettings other) does, I see even more potential issues. For example if you have configured the site to use CDN and the child resource has a CDN URL but the parent resource doesn't, then this will merge the child resource's CDN URL into the parent resource. So in the end it will render the child resource from CDN twice and not render the parent resource. Merging Culture or Version may also have undesired side effects.

As far as I can see, the only thing that's desirable to be merged (and won't be overwritten be the required resource anyway) is Location. The comment that rationalizes the use of .Combine() also only talks about the need to merge location. So I suggest explicitly setting the dependency's location instead of merging all the child resource's properties into it.

[sebastienros]

If something is broken we should get reports quite quickly. Would have been nice to add a few tests as to show what are the expectation (I know the original dev didn't add any, bad person).

[sebastienros]

Does the Style tag helper have the same issue?

[Skrypt]

The new dev didn't either 😢
I will take a look later.

[Skrypt]

@Piedone Also see this one who needs Unit tests.

[Piedone]

What do you mean should I see here?

[Skrypt]

I mean, someone needs to work on adding a unit test for these script and style tag helpers.

[Piedone]

I'm not sure I understand. Why did you approve #15438 without them, then? Otherwise, please create an issue for unit tests.

[Skrypt]

Should we block PR's because of missing unit tests? We never did before. I will add the task.

Added here: #15504

I meant that there is work to do for this too and I need to take a look into this before moving to reviewing other PR's.

[Piedone]

Yes, absolutely. If tests are needed, then a PR is not done until the tests are done.

Thanks! Just to be clear, while Sára is my colleague and works on a lot of OC-related tasks as a Lombiq developer, this was done in her own time without my involvement.

[Skrypt]

I'm not 100% committed that unit test enforcement as you just said. People are doing this on their own free time and sometimes creating unit tests is not their bag of tea either. So, we either need to write them and merge on top of their PR's or merge them and do the unit tests later. I think, for this one it needs to be tested by people using the dev branch. We don't even know what the initial specs we're and why it copied these attributes to the dependency tree (script at least).

All we can do is define the specs now and try to support others if we ever encounter them later on.

[Piedone]

Yeah but then somebody else needs to write the tests in their free time :). I can't comment on the details of this specific PR.