Add support to HashedFilesMixin for ES modules

[gilmrjc]

The current HashedFilesMixin only supports importing static files in css. When importing es modules in javascript files and using ManifestStaticFilesStorage to collect statics, the javascript code stops working because its using a wrong url for the modules needed.

This change already has a PR (​https://github.com/django/django/pull/13843).

[Carlton Gibson]

OK, yes, modules are increasingly important. We should at least see if we can support them. (Initial glance at the PR looks simple enough — too simple? 🤔 :)
Thanks for the report

[Mariusz Felisiak <felisiak.mariusz@…>]

In 781b4424:

Refs #32319 -- Changed HashedFilesMixin to use named groups in patterns.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 91e21836:

Fixed #32319 -- Added ES module support to ManifestStaticFilesStorage.

[GitHub <noreply@…>]

In ba9ced3e:

Fixed #33253 -- Reverted "Fixed #32319 -- Added ES module support to ManifestStaticFilesStorage."

This reverts commit 91e21836f667c784a8a63ab1f18d81f553e679cb.

export and import directives have several syntax variants and not
all of them were properly covered.

Thanks Hervé Le Roy for the report.

[Mariusz Felisiak <felisiak.mariusz@…>]

In b7b3bbc8:

[4.0.x] Fixed #33253 -- Reverted "Fixed #32319 -- Added ES module support to ManifestStaticFilesStorage."

This reverts commit 91e21836f667c784a8a63ab1f18d81f553e679cb.

export and import directives have several syntax variants and not
all of them were properly covered.

Thanks Hervé Le Roy for the report.
Backport of ba9ced3e9a643a05bc521f0a2e6d02e3569de374 from main

[Mariusz Felisiak]

We decided to revert the original patch.

[Michael]

Replying to Mariusz Felisiak <felisiak.mariusz@…>:

In b7b3bbc8:

[4.0.x] Fixed #33253 -- Reverted "Fixed #32319 -- Added ES module support to ManifestStaticFilesStorage."

This reverts commit 91e21836f667c784a8a63ab1f18d81f553e679cb.

export and import directives have several syntax variants and not
all of them were properly covered.

Thanks Hervé Le Roy for the report.
Backport of ba9ced3e9a643a05bc521f0a2e6d02e3569de374 from main

Why is there support for export? I can't find any mention of export with a url here:
​https://developer.mozilla.org/en-US/docs/web/javascript/reference/statements/export
Could you please provide an example of how export with a url works?

Please see my attempt which I am using in my code base:
​https://github.com/django/django/pull/15177

It works with the following import statements:

import { name, draw, reportArea, reportPerimeter } from './modules/square.js';                         
import randomSquare from './modules/square.js';                                                        
import {default as randomSquare} from './modules/square.js';                                           
import { newFunctionName, anotherNewFunctionName } from './modules/module.js';                         
import { function1 as newFunctionName,                                                                 
        function2 as anotherNewFunctionName } from './modules/module.js';                              
import { name, draw, reportArea, reportPerimeter } from './modules/square.js';                         
import { name, draw, reportArea, reportPerimeter } from './modules/circle.js';                         
import { name, draw, reportArea, reportPerimeter } from './modules/triangle.js';                       
import { name as squareName,                                                                           
        draw as drawSquare,                                                                            
        reportArea as reportSquareArea,                                                                
        reportPerimeter as reportSquarePerimeter } from './modules/square.js';                         
                                                                                                       
import { squareName, drawSquare, reportSquareArea, reportSquarePerimeter } from './modules/square.js'; 
import * as Canvas from './modules/canvas.js';                                                         
import { Square } from './modules/square.js';                                                          
import { Square, Circle, Triangle } from './modules/shapes.js';                                        
import colors from './modules/getColors.js';

[Claude Paroz]

I also think that this ticket should more clearly mention why the commit was reverted, that is expose the rationale and failure examples provided by Hervé Le Roy.

[Claude Paroz]

I'd really see this issue going forward. What about re-committing Hervé's patches and creating new release blockers (for 4.2) tickets for the failing use cases?

[Mariusz Felisiak]

Replying to Claude Paroz:

I'd really see this issue going forward. What about re-committing Hervé's patches and creating new release blockers (for 4.2) tickets for the failing use cases?

I'm afraid that introducing a release blocker on purpose is not something that is acceptable. The main issue with this ticket is the extensive way in which import/export syntax is used in JavaScript. As far as I'm aware analyzing JS files with regular expressions is not enough to only sift the import / export statements related with ES modules. Maybe limiting a patch to the .mjs files would solve the issue, hard to say 😞

[Claude Paroz]

Could we report on the ticket the discussion that led to the commit reverts?

[Mariusz Felisiak]

Replying to Claude Paroz:

Could we report on the ticket the discussion that led to the commit reverts?

Most of the short discussion is in this ​PR (I should be more detailed in my comments, sorry.)

[blighj]

I think it is possible to do this with a regex and still work with the kinds of statements found in higlight.js from #33253. You can tighten up what is matched by being more specfic on the url it matches, based on the specs of what a browser will support for a module specifier.
​https://v8.dev/features/modules#specifiers

// Supported:
import {shout} from './lib.mjs';
import {shout} from '../lib.mjs';
import {shout} from '/modules/lib.mjs';
import {shout} from 'https://simple.example/modules/lib.mjs';

For now, module specifiers must be full URLs, or relative URLs starting with /, ./, or ../.

The collectstatic command should not be changing absolute URLs, only relative ones, so we can and an extra requirement to the (?P<url>) matcher so it must start with a dot or forward slash (?P<url>[\.\/].*?)
This limits what the regex matches, so that the code types in highlight.js, and some other issues I've seen in videojs and ace, are no longer matched.
But it doesn't have to worry about all possible values of the import/export part of the expression, which would be very hard to do with regex.

Here is what I'm using in my project for the js patterns

        (
            "*.js",
            (
                (
                    r"""(?P<matched>import(?P<import>[\s\{].*?)\s*from\s*['"](?P<url>[\.\/].*?)["']\s*;)""",
                    """import%(import)s from "%(url)s";""",
                ),
                (
                    r"""(?P<matched>export(?P<exports>[\s\{].*?)\s*from\s*["'](?P<url>[\.\/].*?)["']\s*;)""",
                    """export%(exports)s from "%(url)s";""",
                ),
                (
                    r"""(?P<matched>import\s*['"](?P<url>[\.\/].*?)["']\s*;)""",
                    """import"%(url)s";""",
                ),
            ),
        ),

I'm enforcing the need for a semicolon for extra safety, and I've added support for

import{shout}from './lib.mjs';

which was needed for some js coming out of esbuild.

I don't know if // is supported by browsers for module specifiers, if it is then you'd have to update the regex to not capture those, I haven't needed it for my own projects yet, so I'm keeping my regex simpler for now.

[Keryn Knight]

#34100 (opened by blighj, above) was a marked as a duplicate of this.

[Carlton Gibson]

​Fresh PR here.

This works with the higlight.js file but not the type definitions from this review, PR15058 (review). Since they are a feature of typescript and not javascript, I think that's a fair trade off.

[Carlton Gibson]

It's not clear the proposed patch covers all usages, but it looks OK for common cases.

I think we need to be prepared to say where we're not going to try more complex solutions.

But let's see... 😬

[GitHub <noreply@…>]

In e44d348:

Fixed #32319 -- Added ES module support to ManifestStaticFilesStorage.

Co-authored-by: James Bligh <james.bligh@…>

[Michael]

Moved Post into a new issue, because it seemed to be ignored here.

[Michael]

Replying to blighj:

I'm enforcing the need for a semicolon for extra safety, and I've added support for

Enforcing the ; for "safety" actually breaks import assertions, e.g.:

import l from"/static/skin/skin/x-triangle.min.css"assert{type:"css"};

I think the following 2 steps will make it much for robust and cover much more use cases in modern web development:

1. Remove the ending ; from the regex's
2. Add the extra regex from my previous post.

[Michael]

Thank for fixing the issue, I just came across a mistake in the regex I posted above:

(
                r"""(?P<matched>(?P<import_as>import\s*\*as\s\S)+\s+from\s*["'](?P<url>[./].*?)["']\s*;)""",
                """%(import_as)s from "%(url)s";""",
            ),

Should be (the plus should be for one of more chars of the import as name, not groups, so the plus moved one char to the left:

(
                r"""(?P<matched>(?P<import_as>import\s*\*as\s\S+)\s+from\s*["'](?P<url>[./].*?)["']\s*;)""",
                """%(import_as)s from "%(url)s";""",
            ),