Repository navigation
Only link .css files in assets as stylesheets - #4105
Thanatos9404 wants to merge 3 commits into
Conversation
The assets folder treated any file ending in "css" as a stylesheet, so .scss files were linked in the page head and hot-reloaded as CSS. Check for the .css extension in the initial walk and in hot reload. Fixes plotly#1684.
|
Thanks for submitting this. The team will take a look and follow up. |
T4rk1n
left a comment
T4rk1n
left a comment
There was a problem hiding this comment.
The .css check is right, but it's still a suffix check and only the CSS side got fixed. The JS side next to it still uses endswith("js"), and .mjs is handled by yet another suffix check at render time. The type check is also copied in three places (_walk_assets_directory, plus the add and delete paths in _on_assets_change), so the next file type will have the same problem. I'd rather see one helper that maps a file's extension (os.path.splitext) to how Dash loads it, used by both the walk and hot reload. I'd hold the merge for that.
| self._add_assets_resource(path, full) | ||
| ) | ||
| elif f.endswith("css"): | ||
| elif f.endswith(".css"): |
There was a problem hiding this comment.
The js check just above is still endswith("js"), so a file named nodejs or bundle.cjs in assets gets added as a script tag. Can you move both checks into one helper that uses os.path.splitext(f)[1] against a set of known extensions (.js, .mjs, .css), and call it here and in _on_assets_change? That way a new type only needs adding in one place.
| "url": self.get_asset_url(asset_path), | ||
| "modified": int(modified), | ||
| "is_css": filename.endswith("css"), | ||
| "is_css": filename.endswith(".css"), |
There was a problem hiding this comment.
is_css is the only file type the hot reload payload knows about. Everything else, .mjs included, causes a full reload. If the helper returns the file type, can we send that to the renderer instead of a CSS yes/no?
|
|
||
| if filename not in self._assets_files and not deleted: | ||
| res = self._add_assets_resource(asset_path, filename) | ||
| if filename.endswith("js"): |
There was a problem hiding this comment.
Same endswith("js") here and in the delete branch below. These should use the same helper as the walk, so loading and hot reload can't drift apart again.
_asset_type() maps a file's extension (.js, .mjs, .css) to how Dash loads it. The assets walk, the .mjs check at render time and both hot reload branches use it, so files like nodejs or bundle.cjs are no longer added as scripts. The hot reload payload now sends that type instead of is_css, and the renderer checks for type 'css'.
|
Thanks, done in ff7e293. |
|
Fixes #1684.
Dash decided how to load files in
assets/with suffix checks:endswith("css")linkedtheme.scss(or any name ending in "css") as a stylesheet, andendswith("js")added files such asnodejsorbundle.cjsas scripts. Hot reload repeated the same checks and only told the renderer whether a file was CSS.Now one helper,
_asset_type(), maps the extension (.js,.mjs,.css) to how Dash loads the file. The assets walk, the.mjsmodule check at render time and both hot reload branches use it, and the hot reload payload sends thattypeinstead ofis_css. The renderer swaps the stylesheet in place fortype === 'css'and reloads the page for anything else, as before.One behaviour change: saving a
.scssfile insideassets/now triggers a full page reload, as other non-CSS assets do. Sass sources can be kept outsideassets/or excluded withassets_ignore.Contributor Checklist
is_cssoptionals
CHANGELOG.mdThe two tests in
tests/unit/test_resources.pycover the page head and hot reload (add and delete) and fail without the change. Adash_duocheck with the dev bundle patched the same way confirmed that a CSS edit still swaps the stylesheet without a reload and a.scssedit reloads the page.