[Django] #32319: Add support to HashedFilesMixin for ES modules
Django
-----------------------------------------+------------------------
Reporter: gilmrjc | Owner: nobody
Type: New feature | Status: new
Component: Uncategorized | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/32319>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: gilmrjc | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gilmrjc):
* version: 3.1 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:1>
Django
Reporter: gilmrjc | Owner: gilmrjc
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* owner: nobody => gilmrjc
* status: new => assigned
* component: Uncategorized => contrib.staticfiles
* stage: Unreviewed => Accepted
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:2>
Django
Reporter: gilmrjc | Owner: gilmrjc
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:3>
Django
Reporter: gilmrjc | Owner: gilmrjc
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
* needs_tests: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:4>
Django
Reporter: gilmrjc | Owner: gilmrjc
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"91e21836f667c784a8a63ab1f18d81f553e679cb" 91e21836]:
{{{
#!CommitTicketReference repository=""
revision="91e21836f667c784a8a63ab1f18d81f553e679cb"
Fixed #32319 -- Added ES module support to ManifestStaticFilesStorage.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:6>
Django
Reporter: gilmrjc | Owner: gilmrjc
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"781b44240a06f0c868254f40f36ce46c927f56d1" 781b4424]:
{{{
#!CommitTicketReference repository=""
revision="781b44240a06f0c868254f40f36ce46c927f56d1"
Refs #32319 -- Changed HashedFilesMixin to use named groups in patterns.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:5>
Django
Reporter: gilmrjc | Owner: gilmrjc
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"ba9ced3e9a643a05bc521f0a2e6d02e3569de374" ba9ced3e]:
{{{
#!CommitTicketReference repository=""
revision="ba9ced3e9a643a05bc521f0a2e6d02e3569de374"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:7>
Django
Reporter: gilmrjc | Owner: gilmrjc
Type: New feature | Status: closed
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"b7b3bbc8356b8365f3ab8ed9c8d863b47c18a3d4" b7b3bbc8]:
{{{
#!CommitTicketReference repository=""
revision="b7b3bbc8356b8365f3ab8ed9c8d863b47c18a3d4"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:8>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: closed => new
* has_patch: 1 => 0
* resolution: fixed =>
* stage: Ready for checkin => Accepted
Comment:
We decided to revert the original patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:9>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael):
Replying to [comment:8 Mariusz Felisiak <felisiak.mariusz@…>]:
> In [changeset:"b7b3bbc8356b8365f3ab8ed9c8d863b47c18a3d4" b7b3bbc8]:
> {{{
> #!CommitTicketReference repository=""
revision="b7b3bbc8356b8365f3ab8ed9c8d863b47c18a3d4"
> [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';
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:10>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:11>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:12>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:12 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 😞
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:13>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
Could we report on the ticket the discussion that led to the commit
reverts?
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:14>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:14 Claude Paroz]:
> Could we report on the ticket the discussion that led to the commit
reverts?
Most of the short discussion is in this
[https://github.com/django/django/pull/15058 PR] (I should be more
detailed in my comments, sorry.)
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:15>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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. 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:16>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Keryn Knight):
#34100 (opened by blighj, above) was a marked as a duplicate of this.
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:17>
Django
Reporter: gilmrjc | Owner: gilmrjc
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by blighj):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:18>
Django
Reporter: gilmrjc | Owner: blighj
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: gilmrjc => blighj
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:19>
Django
Reporter: gilmrjc | Owner: blighj
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
[https://github.com/django/django/pull/16241 Fresh PR here].
> This works with the higlight.js file but not the type definitions from
this review, #15058 (review). Since they are a feature of typescript and
not javascript, I think that's a fair trade off.
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:20>
Django
Reporter: gilmrjc | Owner: blighj
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
Comment:
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... 😬
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:21>
Django
Reporter: gilmrjc | Owner: blighj
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"e44d348c99f0a449180399045ac54b3909121a03" e44d348]:
{{{
#!CommitTicketReference repository=""
revision="e44d348c99f0a449180399045ac54b3909121a03"
Fixed #32319 -- Added ES module support to ManifestStaticFilesStorage.
Co-authored-by: James Bligh <james...@silvercloudhealth.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:22>
Django
Reporter: gilmrjc | Owner: blighj
Type: New feature | Status: closed
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael):
I was using my own regex, but removed it now that django support this
natively. But I dont think Django's regex works with the following:
{{{
import*as l from "/static/jsapp/jsapp/dtmod.min.js";import*as h from
"/static/jsapp/jsapp/nummod.min.js";import*as m from
"/static/leave/jsapp/fetcher.min.js";import {BaseComponent as g} from
"/static/wcapp/jsapp/wc-base.min.425310100bce.js";
}}}
As you can see only the 4th import was correctly altered, the first 3
werent even detected, (below is the same as above but placed the imports
on seprate lines for readability):
{{{
import*as l from "/static/jsapp/jsapp/dtmod.min.js";
import*as h from "/static/jsapp/jsapp/nummod.min.js";
import*as m from "/static/leave/jsapp/fetcher.min.js";
import {BaseComponent as g} from "/static/wcapp/jsapp/wc-
base.min.425310100bce.js";
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:23>
Django
Reporter: gilmrjc | Owner: blighj
Type: New feature | Status: closed
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael):
Replying to [comment:16 blighj]:
what is matched by being more specfic on the url it matches, based on the
{{{
import l from"/static/skin/skin/x-triangle.min.css"assert{type:"css"};
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:24>
Django
Reporter: gilmrjc | Owner: blighj
Type: New feature | Status: closed
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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";""",
),
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32319#comment:25>