Re: [Django] #34322: ManifestStaticFilesStorage crashes on commented JavaScript import statements
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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:
> #32319 added module support to `ManifestStaticFilesStorage`. It can crash
> with imports in comments, which bundlers like Webpack can leave in.
>
> Example:
>
> {{{
> //** @type {import("./htmx").HtmxApi} */
> }}}
>
> Leads to:
>
> {{{
> whitenoise.storage.MissingFileError: The file 'example/dist/htmx' could
> not be found with
> <whitenoise.storage.CompressedManifestStaticFilesStorage object at
> 0x16ff630a0>.
>
> The JS file 'example/dist/app.js' references a file which could not be
> found:
> example/dist/htmx
>
> Please check the URL references in this JS file, particularly any
> relative paths which might be pointing to the wrong location.
> }}}
>
> The regex should be adjusted to only select imports that are alone on a
> line, with whitespace.
New description:
#32319 added module support to `ManifestStaticFilesStorage`. It can crash
with imports in comments, which are used for Typescript
([https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html
#import-types docs]) but don't necessarily resolve when code is bundled.
Example [https://github.com/bigskysoftware/htmx/blob/master/src/htmx.js
from htmx]:
{{{
//** @type {import("./htmx").HtmxApi} */
}}}
Leads to:
{{{
whitenoise.storage.MissingFileError: The file 'example/dist/htmx' could
not be found with <whitenoise.storage.CompressedManifestStaticFilesStorage
object at 0x16ff630a0>.
The JS file 'example/dist/app.js' references a file which could not be
found:
example/dist/htmx
Please check the URL references in this JS file, particularly any
relative paths which might be pointing to the wrong location.
}}}
The regex should be adjusted to only select imports that are alone on a
line, with whitespace.
This may be a challenge as comments can be multi-line like:
{{{
/**
* @param {HTMLElement} elt
* @returns {import("./htmx").HtmxTriggerSpecification[]}
*/
}}}
--
Comment (by Adam Johnson):
Added more detail to description about possible failures
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Adam Johnson):
I think it's worth noting that this kind of failure was always possible
with the other replaced regexes. Commented `url()` or `@import` lines in a
CSS file could always fail in a similar way. It seems that the TypeScript
syntax has made it way more likely that there will be commented
unresolvable imports in JavaScript files.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:4>
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Claude Paroz):
I wouldn't be shocked if we would document that imports in comments are
currently not supported in ManifestStaticFilesStorage. Is the situation
worse than when the import directive wasn't supported at all?
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:5>
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Adam Johnson):
In the case I found, imports in comments aren't really controllable in the
toolchain. Either you use the webpack dev mode and output them, or you
remove all comments and minify etc. So in dev you don't want the comments
there.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:6>
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Carlton Gibson):
Adam, do you have any ideas for the way forward? — A regex vs multi-line
comments 😬 — "...you remove all comments..." — is that something we can
require?
Current status is that without some approach we may need to revert
(looking at 4.2b1 next week).
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:7>
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Claude Paroz):
This is in fact just one more occurrence of #21080, so the fact that
`ManifestStaticFileStorage` is broken with commented code, be it in CSS or
JS, is a known fact since some time. Every time we'll extend
`ManifestStaticFileStorage` functionality, we are going to worsen the
situation a bit more.
One possible mitigation could be to condition JS import support to a class
attribute, so it would at least allow people wanting to opt out of this
new functionality to subclass `ManifestStaticFileStorage` to fix any
possible issue with commented imports. I plead to keep this functionality
in 4.2 to allow those writing modern module-based JS in their projects to
safely use `ManifestStaticFileStorage`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:8>
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Mariusz Felisiak):
Replying to [comment:8 Claude Paroz]:
> This is in fact just one more occurrence of #21080, so the fact that
`ManifestStaticFileStorage` is broken with commented code, be it in CSS or
JS, is a known fact since some time. Every time we'll extend
`ManifestStaticFileStorage` functionality, we are going to worsen the
situation a bit more.
I'm happy to close it as a duplicate of #21080. It's a long standing and
well known issue, `ManifestStaticFilesStorage` crashes on references
inside comments.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:9>
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Mariusz Felisiak):
I think documenting this issue is justified as it's been 9 years since it
was opened, see [https://github.com/django/django/pull/16561 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:10>
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 GitHub <noreply@…>):
In [changeset:"bae053d497ba8a8de7e4f725973924bfb1885fd2" bae053d4]:
{{{
#!CommitTicketReference repository=""
revision="bae053d497ba8a8de7e4f725973924bfb1885fd2"
Refs #21080, Refs #34322 -- Added warning to ManifestStaticFilesStorage
docs about paths in comments.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:11>
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"e1c74bf45803cbde28149fbc80d91dc3b244e7fc" e1c74bf4]:
{{{
#!CommitTicketReference repository=""
revision="e1c74bf45803cbde28149fbc80d91dc3b244e7fc"
[4.2.x] Refs #21080, Refs #34322 -- Added warning to
ManifestStaticFilesStorage docs about paths in comments.
Backport of bae053d497ba8a8de7e4f725973924bfb1885fd2 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:12>
Django
statements
-------------------------------------+------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"60be5909a29650a5e7595bbdc4012b0ad0c390af" 60be590]:
{{{
#!CommitTicketReference repository=""
revision="60be5909a29650a5e7595bbdc4012b0ad0c390af"
[4.1.x] Refs #21080, Refs #34322 -- Added warning to
ManifestStaticFilesStorage docs about paths in comments.
Backport of bae053d497ba8a8de7e4f725973924bfb1885fd2 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:13>
Django
statements
Reporter: Adam Johnson | Owner: nobody
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Mariusz Felisiak):
* status: new => closed
* resolution: => duplicate
* severity: Release blocker => Normal
Comment:
Duplicate of #21080.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:14>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Adam Johnson):
I'm not really happy about closing this with just a warning. The previous
issue covered references inside CSS comments - something that is rare. The
new issue with JS comments is something we can expect to be very common.
Currently Django 4.2 will make using two fairly common things - htmx (non-
minified) and WhiteNoise - crash `collectstatic` with an obscure error
message.
We can do better, we have `django.utils.jslex` which can be used to
identify import statements, rather than relying on fragile regexes. I
started working on a patch but didn't get far, I don't get much time for
open source at the moment.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:15>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Claude Paroz):
Adam, is your current patch shareable?
If the current state is not acceptable for 4.2, we could still make that
feature togglable with an opt-in/opt-out flag on the class as I suggested
on comment:8.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:16>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "js-commented-regions.patch" added.
PoC patch to avoid commented regions
--
Ticket URL: <https://code.djangoproject.com/ticket/34322>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Adam Johnson):
I just attached the exploratory patch. I'll explain the status below, but
first some fresh shower thoughts on impact.
This bug probably affects many projects. Lots of projects use a bunch of
JavaScript for their frontend, and the number of packages a typical JS
project *probably* means at least one uses TypeScript import comments.
It's also mostly outside of users' control - they need React, it pulls in
50 other things, they can't easily remove the comments.
Despite the impact, I think it's also *unlikely* to be detected by many
people in Django's beta phase. The standard advice is to test with your
project's test suite, which would does not include `collectstatic`. (In
fact, I only "accidentally" ran collectstatic, it wasn't on my checklist.)
Case in point: the CSS source map change to `HashedFilesMixin` in Django
4.1 (#33353) broke Django Rest Framework, but no one found that until the
final release. The fix was simple in that case: add the missing source map
file to DRF (https://github.com/encode/django-rest-framework/pull/8591).
But in this case, there's no simple fix. Users are unlikely to be able to
edit out every typescript comment from all their libraries.
Okay, on the patch I started on... it's quite exploratory really. It tries
to find all commented regions within the JS file, and skip processing
import statements within those. But I realized I would *really* need to
expand the approach to all kinds of string literals that might mention the
word "import", and then I got to the point of realizing that regexes just
won't cut it (not a huge surprise in retrospect).
To avoid all false positives, we need to properly lex/tokenize the JS so
we can partially-parse only real `import` statements. Luckily Django
already has a JS lexer, but it would still be some work.
The patch currently fails with JS regex literals that contain the word
"import", or quote marks/backticks (because they match a string start):
{{{
const importRe = /import/;
const doubleQuoteRe = /"/;
}}}
`django.utils.jslex` should be able to parse past those.
I would suggest that we *stop* processing files with regexes, at least
JavaScript files, and instead tokenize with `jslex`. Then we can process
the rules individually.
An optimization would be to skip tokenzing files that do not contain the
words `sourceMappingURL`, `import`, or `export` (which a first-pass regex
could detect). This would probably be required to avoid noticeable
slowdown, since the tokenization process is almost certainly slower than
the current regex approach.
This change feels quite heavy though, I'm not sure it's something we'd
want to do in a beta release?
I was using a test project with an `app.js` containing:
{{{
// These should not be processed
// @returns {import("./non-existent-1").something}
/* @returns {import("./non-existent-2").something} */
'import("./non-existent-3")'
"import('./non-existent-4')"
`import("./non-existent-5")`
r = /import/;
// This should be processed
import example from "./module.js";
}}}
And an empty file `module.js` next to it.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:17>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Claude Paroz):
> This change feels quite heavy though, I'm not sure it's something we'd
want to do in a beta release?
Indeed, it seems late for such a change.
Here's a possible opt-in implementation of that new functionality, without
docs, just for exploration and opinion:
[https://github.com/claudep/django/commit/0eaf4f3542a64c8df66dc92e8574ab45d16053fd]
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:18>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Mariusz Felisiak):
Replying to [comment:18 Claude Paroz]:
> > This change feels quite heavy though, I'm not sure it's something
we'd want to do in a beta release?
>
> Indeed, it seems late for such a change.
>
> Here's a possible opt-in implementation of that new functionality,
without docs, just for exploration and opinion:
[https://github.com/claudep/django/commit/0eaf4f3542a64c8df66dc92e8574ab45d16053fd]
Agreed, we can change this to an experimental opt-in feature. Thanks both!
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:19>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Sebastiaan Zeeff):
I'm also running into this problem and I don't thinks it's confined to
comments. Unfortunately, it only comes up when you really start working
with the 4.2b1 instead of just running test suites without running
`collectstatic`.
I added a comment to the old issue (ten years old), but since discussion
is going on here as well, here's a link:
https://code.djangoproject.com/ticket/21080#comment:26
I fear that this change in scanning for imports may break a lot more
projects and a lot of those project are probably using third-party
JavaScript/CSS dependencies that they can't easily change to circumvent
false positives in the import scanning procedure of Django. The third-
party package I'm using includes a widget that shows documentation,
including examples of imports in JavaScript strings, and it's probably not
the only third-party package that includes import examples somewhere in
files (either in comments or in the "empty states" of widgets that have
not been initialized yet).
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:20>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Mariusz Felisiak):
Replying to [comment:20 Sebastiaan Zeeff]:
> I'm also running into this problem and I don't thinks it's confined to
comments. Unfortunately, it only comes up when you really start working
with the 4.2b1 instead of just running test suites without running
`collectstatic`.
>
> I added a comment to the old issue (ten years old), but since discussion
is going on here as well, here's a link:
https://code.djangoproject.com/ticket/21080#comment:26
>
> I fear that this change in scanning for imports may break a lot more
projects and a lot of those project are probably using third-party
JavaScript/CSS dependencies that they can't easily change to circumvent
false positives in the import scanning procedure of Django. The third-
party package I'm using includes a widget that shows documentation,
including examples of imports in JavaScript strings, and it's probably not
the only third-party package that includes import examples somewhere in
files (either in comments or in the "empty states" of widgets that have
not been initialized yet).
Thanks for your comment. What do you think about
https://code.djangoproject.com/ticket/34322#comment:18?
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:21>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Klaas van Schelven):
> This bug probably affects many projects. Lots of projects use a bunch of
JavaScript for their frontend, and the number of packages a typical JS
project *probably* means at least one uses TypeScript import comments.
It's also mostly outside of users' control - they need React, it pulls in
50 other things, they can't easily remove the comments.
+1 on this sentiment
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:22>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Sebastiaan Zeeff):
Replying to [comment:21 Mariusz Felisiak]:
> Replying to [comment:20 Sebastiaan Zeeff]:
> > (snipped for brevity)_
> Thanks for your comment. What do you think about
https://code.djangoproject.com/ticket/34322#comment:18?
It's a good option for end-users, although they'll have to discover the
option. The `staticfiles` error itself is probably going to be a bit
confusing, especially to less experienced users.
It's a bit more difficult for package/plugin maintainers, as they can't
control the settings of the users using their package. For my current
client, I'm maintaining a package that vendors a components library and
integrates it into Django's form system to make using these widgets in
forms and generic edit views easy. It's used by a lot of projects, which
means all of them will have to make sure that they're using the opt-in
when upgrading to Django 4.2 LTS (which is probably going to be mandatory
in this large organisation).
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:23>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Mariusz Felisiak):
It's a good option for end-users, although they'll have to discover the
option. The staticfiles error itself is probably going to be a bit
confusing, especially to less experienced users.
The proposition is to make it opt-in not opt-out, so they'll have to
discover the option to turn it on. Moreover, it will be documented as
experimental with warning (already documented) about comments.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:24>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Normal | Resolution: duplicate
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 Sebastiaan Zeeff):
Replying to [comment:24 Mariusz Felisiak]:
> It's a good option for end-users, although they'll have to discover the
option. The staticfiles error itself is probably going to be a bit
confusing, especially to less experienced users.
>
> The proposition is to make it opt-in not opt-out, so they'll have to
discover the option to turn it on. Moreover, it will be documented as
experimental with warning (already documented) about comments.
That means I misread the earlier conversation. I interpreted it as "it's
to late to make major changes to the behavior that's currently present in
the beta, but there's an opt-in patch that will solve issues for people
who are experience issues with the new import detection".
Apologies for taking up your time.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:25>
Django
statements
-------------------------------------+------------------------------------
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* status: closed => new
* resolution: duplicate =>
* severity: Normal => Release blocker
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:26>
Django
statements
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | 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 Mariusz Felisiak):
* owner: nobody => Mariusz Felisiak
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:27>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/16656 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:28>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"e10c1688f96e3b2d202fe401472b7b25f6105969" e10c1688]:
{{{
#!CommitTicketReference repository=""
revision="e10c1688f96e3b2d202fe401472b7b25f6105969"
Fixed #34322 -- Made ES module support to ManifestStaticFilesStorage
optional.
Co-authored-by: Author: Claude Paroz <cla...@2xlibre.net>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:29>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"f2923306f152ed4e385e0544f8bc615d7d65c797" f2923306]:
{{{
#!CommitTicketReference repository=""
revision="f2923306f152ed4e385e0544f8bc615d7d65c797"
[4.2.x] Fixed #34322 -- Made ES module support to
ManifestStaticFilesStorage optional.
Co-authored-by: Author: Claude Paroz <cla...@2xlibre.net>
Backport of e10c1688f96e3b2d202fe401472b7b25f6105969 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:30>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam Johnson):
I have learned that these comments are part of the JSDoc standard:
https://jsdoc.app/
Also a data point, Svelte is moving its process to use such JSDoc comments
over TypeScript, to avoid the build process:
https://github.com/sveltejs/svelte/pull/8569 . So such comments may become
more common if JavaScirpt developers decide to follow this popular
project’s approach.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:31>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
I've put together a [https://github.com/django/django/pull/19551 PR 19551]
to use JsLex as per Adam's suggestion in comment:17
It seems to work quite well, the tests are all passing and it seems a more
robust approach to the problem. Takes care of the issues with comments and
some of the other edge cases that the regexes was struggling with. I've
left in the optin behaviour of the current implenetation, but with enough
feedback we could consider making it the default.
I'm not sure on the etiquette of associating a PR with a closed ticket.
Let me know if I need to do something differently there.
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:32>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
Cleanup/optimization | Status: new
Component: contrib.staticfiles | Version: dev
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: blighj (added)
* resolution: fixed =>
* version: 4.2 => dev
--
Ticket URL: <https://code.djangoproject.com/ticket/34322#comment:33>
Django
statements
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
Type: Bug | Status: closed
Component: contrib.staticfiles | Version: 4.2
Severity: Release blocker | Resolution: fixed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Please don't reopen closed tickets. Your works should point to the #21080.