Opened 4 months ago
Closed 3 months ago
#35553 closed Bug (fixed)
HashedFilesMixin for ES modules does not work with `import*as ...` syntax
Reported by: | Michael | Owned by: | Farhan Ali |
---|---|---|---|
Component: | contrib.staticfiles | Version: | 5.0 |
Severity: | Normal | Keywords: | Manifest Static Files Storage, javascript module scripts |
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
Django's regex does not work 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";
This regex handles the missing case:
( r"""(?P<matched>(?P<import_as>import\s*\*as\s\S+)\s+from\s*["'](?P<url>[./].*?)["']\s*;)""", """%(import_as)s from "%(url)s";""", ),
Change History (13)
comment:1 by , 4 months ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
follow-up: 3 comment:2 by , 4 months ago
Hi, no it is not invalid, that is the result of minification, removing any extra whitespace, most production system will serve minified files, so it's much more likely to not have the space.
comment:3 by , 4 months ago
Resolution: | invalid |
---|---|
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
Replying to Michael:
Hi, no it is not invalid, that is the result of minification, removing any extra whitespace, most production system will serve minified files, so it's much more likely to not have the space.
Ah, TIL 😁 confirmed that this is the output of many minifiers
Confirmed that your suggested patch works for me, here is also a test:
-
tests/staticfiles_tests/project/documents/cached/module.js
diff --git a/tests/staticfiles_tests/project/documents/cached/module.js b/tests/staticfiles_tests/project/documents/cached/module.js index 7764e740d6..30ca25e9b6 100644
a b 2 2 import rootConst from "/static/absolute_root.js"; 3 3 import testConst from "./module_test.js"; 4 4 import * as NewModule from "./module_test.js"; 5 import*as m from"./module_test.js"; 5 6 import { testConst as alias } from "./module_test.js"; 6 7 import { firstConst, secondConst } from "./module_test.js"; 7 8 import { -
tests/staticfiles_tests/test_storage.py
diff --git a/tests/staticfiles_tests/test_storage.py b/tests/staticfiles_tests/test_storage.py index 469d5ec690..956341a858 100644
a b class TestCollectionJSModuleImportAggregationManifestStorage(CollectionTestCase) 674 674 675 675 def test_module_import(self): 676 676 relpath = self.hashed_file_path("cached/module.js") 677 self.assertEqual(relpath, "cached/module. 55fd6938fbc5.js")677 self.assertEqual(relpath, "cached/module.d16a17156de1.js") 678 678 tests = [ 679 679 # Relative imports. 680 680 b'import testConst from "./module_test.477bbebe77f0.js";', … … class TestCollectionJSModuleImportAggregationManifestStorage(CollectionTestCase) 686 686 b'const dynamicModule = import("./module_test.477bbebe77f0.js");', 687 687 # Creating a module object. 688 688 b'import * as NewModule from "./module_test.477bbebe77f0.js";', 689 # Creating a minified module object. 690 b'import*as m from "./module_test.477bbebe77f0.js";', 689 691 # Aliases. 690 692 b'import { testConst as alias } from "./module_test.477bbebe77f0.js";', 691 693 b"import {\n"
Would you like to raise a PR?
comment:4 by , 4 months ago
Summary: | HashedFilesMixin for ES modules does not work with `import * as ...` syntax → HashedFilesMixin for ES modules does not work with `import*as ...` syntax |
---|
follow-up: 6 comment:5 by , 4 months ago
Okay great, thanks for handling it. I dont know what "TIL" means, but cheers!
comment:6 by , 4 months ago
Replying to Michael:
Okay great, thanks for handling it. I dont know what "TIL" means, but cheers!
Sorry it's short for "Today I learned"
comment:7 by , 4 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 4 months ago
output of this produces import *as m from "./module_test.477bbebe77f0.477bbebe77f0.js";
for me.
I don't know why.
Should not it be import *as m from "./module_test.477bbebe77f0.js";
comment:9 by , 4 months ago
-
django/contrib/staticfiles/storage.py
diff --git a/django/contrib/staticfiles/storage.py b/django/contrib/staticfiles/storage.py index 85172ea42d..394975c9de 100644
a b class HashedFilesMixin: 73 73 r"""(?P<matched>import\(["'](?P<url>.*?)["']\))""", 74 74 """import("%(url)s")""", 75 75 ), 76 ( 77 r"""(?P<matched>(?P<import_as>import\s*\*as\s\S+)\s+from\s*["'](?P<url>[./].*?)["']\s*;)""", 78 """%(import_as)s from "%(url)s";""", 79 ), 76 80 ), 77 81 ) 78 82 patterns = ( … … class HashedFilesMixin: 287 291 288 292 # where to store the new paths 289 293 hashed_files = {} 290 291 294 # build a list of adjustable files 292 295 adjustable_paths = [ 293 296 path for path in paths if matches_patterns(path, self._patterns) 294 297 ] 295 296 298 # Adjustable files to yield at end, keyed by the original path. 297 299 processed_adjustable_paths = {} 298 300 -
tests/staticfiles_tests/project/documents/cached/module.js
diff --git a/tests/staticfiles_tests/project/documents/cached/module.js b/tests/staticfiles_tests/project/documents/cached/module.js index 7764e740d6..602561798f 100644
a b 2 2 import rootConst from "/static/absolute_root.js"; 3 3 import testConst from "./module_test.js"; 4 4 import * as NewModule from "./module_test.js"; 5 import *as m from "./module_test.js"; 5 6 import { testConst as alias } from "./module_test.js"; 6 7 import { firstConst, secondConst } from "./module_test.js"; 7 8 import { -
tests/staticfiles_tests/test_storage.py
diff --git a/tests/staticfiles_tests/test_storage.py b/tests/staticfiles_tests/test_storage.py index dc8607a307..6290d9d51a 100644
a b class TestCollectionJSModuleImportAggregationManifestStorage(CollectionTestCase) 643 643 644 644 def test_module_import(self): 645 645 relpath = self.hashed_file_path("cached/module.js") 646 self.assertEqual(relpath, "cached/module. 55fd6938fbc5.js")646 self.assertEqual(relpath, "cached/module.0415cd43ac63.js") 647 647 tests = [ 648 648 # Relative imports. 649 649 b'import testConst from "./module_test.477bbebe77f0.js";', … … class TestCollectionJSModuleImportAggregationManifestStorage(CollectionTestCase) 655 655 b'const dynamicModule = import("./module_test.477bbebe77f0.js");', 656 656 # Creating a module object. 657 657 b'import * as NewModule from "./module_test.477bbebe77f0.js";', 658 # Creating a minified module object. 659 b'import*as m from "./module_test.477bbebe77f0.js";', 658 660 # Aliases. 659 661 b'import { testConst as alias } from "./module_test.477bbebe77f0.js";', 660 662 b"import {\n"
comment:10 by , 3 months ago
Has patch: | set |
---|
comment:11 by , 3 months ago
Patch needs improvement: | set |
---|
comment:12 by , 3 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Replying to Michael:
This doesn't look valid to me, shouldn't it be
import * as
notimport*as
?