Opened 4 years ago

Closed 20 months ago

Last modified 4 weeks ago

#32319 closed New feature (fixed)

Add support to HashedFilesMixin for ES modules

Reported by: gilmrjc Owned by: blighj
Component: contrib.staticfiles Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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).

Change History (25)

comment:1 by gilmrjc, 4 years ago

Version: 3.1master

comment:2 by Carlton Gibson, 4 years ago

Component: Uncategorizedcontrib.staticfiles
Owner: changed from nobody to gilmrjc
Status: newassigned
Triage Stage: UnreviewedAccepted

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

comment:3 by Mariusz Felisiak, 3 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:4 by Mariusz Felisiak, 3 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 781b4424:

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

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 91e21836:

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

comment:7 by GitHub <noreply@…>, 3 years ago

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.

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

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

comment:9 by Mariusz Felisiak, 3 years ago

Has patch: unset
Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted

We decided to revert the original patch.

in reply to:  8 comment:10 by Michael, 3 years ago

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';

comment:11 by Claude Paroz, 3 years ago

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.

comment:12 by Claude Paroz, 2 years ago

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?

in reply to:  12 comment:13 by Mariusz Felisiak, 2 years ago

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 😞

comment:14 by Claude Paroz, 2 years ago

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

in reply to:  14 comment:15 by Mariusz Felisiak, 2 years ago

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.)

comment:16 by blighj, 21 months ago

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.

Last edited 21 months ago by blighj (previous) (diff)

comment:17 by Keryn Knight, 21 months ago

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

comment:18 by blighj, 21 months ago

Has patch: set

comment:19 by Mariusz Felisiak, 21 months ago

Owner: changed from gilmrjc to blighj
Status: newassigned

comment:20 by Carlton Gibson, 21 months ago

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.

Last edited 21 months ago by Carlton Gibson (previous) (diff)

comment:21 by Carlton Gibson, 20 months ago

Triage Stage: AcceptedReady for checkin

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... 😬

comment:22 by GitHub <noreply@…>, 20 months ago

Resolution: fixed
Status: assignedclosed

In e44d348:

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

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

comment:23 by Michael, 3 months ago

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

Last edited 4 weeks ago by Michael (previous) (diff)

in reply to:  16 comment:24 by Michael, 3 months ago

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.
Last edited 3 months ago by Michael (previous) (diff)

comment:25 by Michael, 4 weeks ago

Last edited 4 weeks ago by Michael (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top