[Django] #36969: collectstatic post-processing with support_js_module_import_aggregation breaks on certain javascript files

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
------------------------+-----------------------------------------------
Reporter: blighj | Type: Bug
Status: new | Component: contrib.staticfiles
Version: 6.0 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+-----------------------------------------------
The regular expressions to find import/export statements in javascript can
work in common cases but still have plenty of failure modes. A lot of the
failures were in comments, which were getting tracked in #21080. But there
will always remain others, the complexity space of javascript is more than
the regexs can handle. Any code with import like statements in strings
will break it for sure.

- https://code.djangoproject.com/ticket/21080#comment:26
- https://code.djangoproject.com/ticket/35371
- https://code.djangoproject.com/ticket/34322#comment:17
- https://code.djangoproject.com/ticket/32849

To my mind this is a bug, where potentially the only viable solution is a
lexer/parser as proposed by Adam in the thrid ticket linked. Which could
merit a DEP for a bug!

Or it might just need some sort of update to the docs to articulate
Carlton's point in this comment
https://code.djangoproject.com/ticket/32319#comment:21
It's not clear the [regex approach] 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... 😬

In either case I think it is usefull to have one ticket that tracs all
these regex failures in one place, rather than having multiple tickets
that are sort of duplicates of this 'regex not working' issue..

There is the package django-manifeststaticfiles-enhanced which provides
the lexer solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/36969>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files

-------------------------------------+------------------------------------
Reporter: blighj | Owner: (none)

Type: Bug | Status: new
Component: contrib.staticfiles | Version: 6.0

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+------------------------------------
Changes (by Favorite):

* needs_tests: 0 => 1
* stage: Unreviewed => Accepted

Comment:

Yeah, I 100% agree with that. I'm going to mark the triage status as
accepted just because the answer to the issue ''shouldn't'' just be do
nothing. I personally won't assign this to myself, but I will suggest to
whoever wants to do it to look into merging the functionality from the
aforementioned package, django-mainfeststaticfiles-enhanced! Good luck,
everyone!
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:1>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
-------------------------------------+------------------------------------
Reporter: blighj | Owner: (none)
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Comment (by kurtmoll):

Hello,

Can I take a look at this issue? I'll start right away by adding to the
test-suite the failures from #21080. Once the commit for the failing
regexes has been provided, I'll see how to integrate the lexer from
`django-manifeststaticfiles-enhanced`, even though discussion is still
needed to agree upon adding such complexity to the codebase (maybe let an
extension handles that instead and keep simple regexes).
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:2>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
-------------------------------------+------------------------------------
Reporter: blighj | Owner: (none)
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by kurtmoll):

What Django is trying to do with post_process is equivalent to a linker/JS
packer, ie rewriting references/relocating. So yes, putting that in Django
source code will be rejected as it can only grow. Why not use `esbuild`
instead of doing it with the regexes? See for example:
https://esbuild.github.io/api/#conditions.
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:3>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
-------------------------------------+------------------------------------

Reporter: blighj | Owner: kurtmoll
Type: Bug | Status: assigned

Component: contrib.staticfiles | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Changes (by kurtmoll):

* owner: (none) => kurtmoll
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:4>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
-------------------------------------+------------------------------------
Reporter: blighj | Owner: kurtmoll
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Comment (by blighj):

I'm not sure if you are proposing adding esbuild to a standard `pip
install django`, but in case you are I doubt that will be a runner. As it
stands the project is heavily against external python libs, nevermind libs
in other languages.

I wouldn't jump in on both feet with a PR for this, there was mention
already of requring a DEP for a lexer based approach. So I'd wait until
there was at least more discussion on this issue before going to a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:5>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
-------------------------------------+------------------------------------
Reporter: blighj | Owner: kurtmoll
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Comment (by kurtmoll):

@blighj, You are right. Third party dependencies is a big no as this
dependency will not be aligned with compatibility, stability and security
concerns of Django. On the other hand, adding a lexer is not a great idea
as it will often break and reimplement part of a JS bundler. So the only
thing to do on my side is to put new tests that fail in
`django/tests/staticfiles_tests` and move on to another issue to let you,
seasoned contributors, discuss the topic further. Thank you for your
answer BTW.
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:6>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
-------------------------------------+------------------------------------

Reporter: blighj | Owner: (none)
Type: Bug | Status: new

Component: contrib.staticfiles | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Changes (by kurtmoll):

* owner: kurtmoll => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:7>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
-------------------------------------+------------------------------------
Reporter: blighj | Owner: (none)
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Comment (by kurtmoll):

I added some failing cases to `test_storage.py`. They fail at the
`setUp()` stage though.
https://github.com/v01dXYZ/django/commit/8f2f253d5fe8361c6c0ec95ce33c8881a2cfb6ca

I didn't dare to open a PR as I don't know if it would appreciated to open
a PR only to have logs and nothing meant to get merged.
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:8>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files

--------------------------------------+------------------------------------
Reporter: blighj | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 6.0

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Jacob Walls):

* component: contrib.staticfiles => Documentation
* needs_tests: 1 => 0
* type: Bug => Cleanup/optimization

Comment:

Thanks for helping us collect these in one place.

> Or it might just need some sort of update to the docs to articulate
Carlton's point in this comment
https://code.djangoproject.com/ticket/32319#comment:21

I think some cases will fall on either side of the line. That's why I'm
leaving the tickets you linked open for now, as there could be an
incremental, well-scoped fix that would be acceptable?

I'll frame this ticket as a documentation update for now. There, we could
mention your package as a place to turn when these edge cases bite.

Then we would either close this ticket as "Fixed", or as
"needsnewfeatureprocess" pending this comment:

> To my mind this is a bug, where potentially the only viable solution is
a lexer/parser as proposed by Adam in the thrid ticket linked. Which could
merit a DEP for a bug!

Anyone who wants to gather consensus for a DEP or sound out folks on the
new-features repo won't need to wait for the docs tweak.
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:9>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
--------------------------------------+------------------------------------
Reporter: blighj | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Old description:

> The regular expressions to find import/export statements in javascript
> can work in common cases but still have plenty of failure modes. A lot of
> the failures were in comments, which were getting tracked in #21080. But
> there will always remain others, the complexity space of javascript is
> more than the regexs can handle. Any code with import like statements in
> strings will break it for sure.
>
> - https://code.djangoproject.com/ticket/21080#comment:26
> - https://code.djangoproject.com/ticket/35371
> - https://code.djangoproject.com/ticket/34322#comment:17
> - https://code.djangoproject.com/ticket/32849
>

> To my mind this is a bug, where potentially the only viable solution is a
> lexer/parser as proposed by Adam in the thrid ticket linked. Which could
> merit a DEP for a bug!
>

> Or it might just need some sort of update to the docs to articulate
> Carlton's point in this comment
> https://code.djangoproject.com/ticket/32319#comment:21

> It's not clear the [regex approach] 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... 😬
>
> In either case I think it is usefull to have one ticket that tracs all
> these regex failures in one place, rather than having multiple tickets
> that are sort of duplicates of this 'regex not working' issue..
>
> There is the package django-manifeststaticfiles-enhanced which provides
> the lexer solution.

New description:

The regular expressions to find import/export statements in javascript can
work in common cases but still have plenty of failure modes. A lot of the
failures were in comments, which were getting tracked in #21080. But there
will always remain others, the complexity space of javascript is more than
the regexs can handle. Any code with import like statements in strings
will break it for sure.

- https://code.djangoproject.com/ticket/21080#comment:26
- https://code.djangoproject.com/ticket/35371
- https://code.djangoproject.com/ticket/34322#comment:17

To my mind this is a bug, where potentially the only viable solution is a
lexer/parser as proposed by Adam in the thrid ticket linked. Which could
merit a DEP for a bug!

Or it might just need some sort of update to the docs to articulate
Carlton's point in this comment
https://code.djangoproject.com/ticket/32319#comment:21

It's not clear the [regex approach] 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... 😬

In either case I think it is usefull to have one ticket that tracs all
these regex failures in one place, rather than having multiple tickets
that are sort of duplicates of this 'regex not working' issue..

There is the package django-manifeststaticfiles-enhanced which provides
the lexer solution.

--

Comment (by blighj):

Removed #32849 as it is not really a case of the regex's failing on
supported statements.
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:10>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
--------------------------------------+------------------------------------
Reporter: blighj | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by muzman123):

I see no one is assigned to this ticket and would like to take a stab at
the documentation update mentioned in comment:9. I'll add a note
clarifying that the regex approach covers common cases but has known
limitations, and mention django-manifeststaticfiles-enhanced as an
alternative for edge cases
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:11>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
--------------------------------------+------------------------------------
Reporter: blighj | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by blighj):

It is unusual to mention third party packages in the docs. I'd recommend
against that part of any update.
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:12>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files

-------------------------------------+------------------------------------
Reporter: blighj | Owner: blighj
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 6.0

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+------------------------------------
Changes (by blighj):

* component: Documentation => contrib.staticfiles
* owner: (none) => blighj

* status: new => assigned

* type: Cleanup/optimization => Bug

Comment:

I've done some futher exploring, and two of the three cases, (and a lot of
the issues I've come across in my own experience), are covered by the
import in quotes problems. The following regex would fix that, so
potentially we could use that and not have to update the docs at all. What
do you thing Jacob?

I've tested the regex's against the benchmark projects I use and it is
doing the job. During that testing I did find an issue on the xstate js
lib, which wasn't directly caused by the change, but in discoverying a fix
for it you get a fix for 35371 too. So we can move this pretty far along
with just regexs, way further than I first suspected.

I've moved the ticket back to bug and assigned myself the owner for now.
But I'll hold off on a PR until I get your input.


{{{
_css_ignored_re = _lazy_re_compile(
r"/\*.*?\*/" # block comment
r"|'(?:[^'\\]|\\.)*'" # single-quoted string
r'|"(?:[^"\\]|\\.)*"', # double-quoted string
re.DOTALL,
)
_js_ignored_re = _lazy_re_compile(
r"/\*.*?\*/" # block comment
r"|//[^\n]*" # line comment
r"|'(?:[^'\\]|\\.)*'" # single-quoted string
r'|"(?:[^"\\]|\\.)*"' # double-quoted string
r"|`(?:[^`\\]|\\.)*`", # template literal
re.DOTALL,
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:13>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files

-------------------------------------+-------------------------------------

Reporter: blighj | Owner: blighj
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:

| Unreviewed

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* stage: Accepted => Unreviewed

--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:14>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
-------------------------------------+------------------------------------

Reporter: blighj | Owner: blighj
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 6.0
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------

Changes (by Jacob Walls):

* stage: Unreviewed => Accepted

Comment:

Sounds promising, happy to see it! Will you include a link to your
benchmarK?
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:15>

[Django]

#36969: collectstatic post-processing with support_js_module_import_aggregation
breaks on certain javascript files
-------------------------------------+------------------------------------
Reporter: blighj | Owner: blighj
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 6.0
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 blighj):

* has_patch: 0 => 1

Comment:

The list of sites in the benchmark is on this issue
https://github.com/blighj/django-manifeststaticfiles-enhanced/issues/34,
but the actual benchmark code is not published anywhere. What I did was
crawl sites from built with django and built with wagtail, find ones that
had hashing in asset names and then find their staticfiles manifest and
curl all the assets to my local machine. That leaves me with a folder with
a bare django project and 50 subfolders of different assets, then the
benchmark script changes the STATICFILES_DIRS and STORAGES setting to test
the different backends against the different projects timing three runs
for each different configuration.
--
Ticket URL: <https://code.djangoproject.com/ticket/36969#comment:16>