Uh oh!
There was an error while loading. Please reload this page.
Issue 480 - Add .map file extension support to dash - #478
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Marc-Andre-Rivet
commented
Dec 6, 2018
@T4rk1n I've created an issue to track this PR in a more normal manner and linking it above in the description. Also updated the changelog. As it should have been done from the start :) |
Marc-Andre-Rivet
commented
Dec 6, 2018
| is_dynamic_resource = ( | ||
| 'dynamic' in resource and | ||
| resource['dynamic'] is True | ||
| ) |
There was a problem hiding this comment.
Not sure what the preferred indentation pattern is for Python -- went with something that seemed natural with the syntax
There was a problem hiding this comment.
No () and single line, we prefer explicit: if 'dynamic' in resources and resource.get('dynamic'). Also .get over [key] to avoid unnecessary KeyErrors. Only use is for comparing pointer references, ie is None to assert null and not just falsy.
There was a problem hiding this comment.
Did this because the line is too long for flake8's taste.
There was a problem hiding this comment.
How do I one line this and make flake8 happy? Apart from an actual function, etc.
There was a problem hiding this comment.
Break them using \ at the end of the line.
There was a problem hiding this comment.
Getting the same error from pylint
There was a problem hiding this comment.
And now this.. dash/dash.py:355:4: R0912: Too many branches (13/12) (too-many-branches
I guess the additional if triggered this rule.
There was a problem hiding this comment.
I prefer if cond to is True, is True is when you need to assert that it the actual True and not another type that evaluate to True in a condition like a string.
There was a problem hiding this comment.
Ah! Misread! No need for the explicit comparison... all good haha
There was a problem hiding this comment.
Add # pylint: disable=too-many-branches to the start of the function.
Uh oh!
There was an error while loading. Please reload this page.
| )) | ||
| ) | ||
| if not is_dynamic_resource: | ||
| srcs.append(path) |
| full = os.path.join(current, f) | ||
| print(f) |
There was a problem hiding this comment.
Oups! Will remove
| for s in all_resources: | ||
| filtered_resource = {} | ||
| if 'dynamic' in s: | ||
| filtered_resource['dynamic'] = s['dynamic'] |
There was a problem hiding this comment.
dynamic property needs to survive the sanitation process -- adding it to the list
rmarren1
left a comment
There was a problem hiding this comment.
I made a few code style suggestions -- nothing critical. Also I made a few comments on previous discussions.
I'm also curious what the motivation behind 'dynamic' is? Do the .map files change at run-time independently of the bundles? I always thought they were static content, just static content that shouldn't be added to the <script> tag of a Dash app. Although, I am not sure what a good short word for that is which can be used as a variable name.
| resource.get('dynamic') | ||
| if 'relative_package_path' in resource: | ||
| if isinstance(resource['relative_package_path'], str): |
There was a problem hiding this comment.
An alternate way might solve the too-many-branches 🐫
paths = resource['relative_package_path']
paths = [paths] if isinstance(paths, str) else paths
for rel_path in paths:
... the logic that is repeated twice ...
There was a problem hiding this comment.
Sure.. I'm in, if only because it makes me learn the language haha!
| srcs = [] | ||
| for resource in resources: | ||
| is_dynamic_resource = 'dynamic' in resource and \ |
There was a problem hiding this comment.
perhaps resource.get('dynamic', False) so it is not split
There was a problem hiding this comment.
dictionary entry with default I guess?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| )) | ||
| else: | ||
| for rel_path in resource['relative_package_path']: | ||
| self.registered_paths[resource['namespace']]\ |
There was a problem hiding this comment.
@rmarren1 Not sure this is considered correct (escaping with '' in the middle of a chain (as opposed to an 'if' as I've done elsewhere)
There was a problem hiding this comment.
Do you mean using the \? That is standard to do
| # track the registered packages | ||
| self.registered_paths[namespace].add(relative_package_path) | ||
| module_path = os.path.join( |
There was a problem hiding this comment.
As suggested, moved the side-effect outside of the path calculator and calling it directly below
Good question. This is 'dynamic' in the same sense as Babel and Webpack defines it -- these files are loaded on a "per need" basis, or dynamically -- not to be confused with the other meaning of dynamic where the files would change at runtime. In that sense 'dynamic' really is the correct word vs. static, as opposed to say 'async'. This is useful for source maps but also when splitting code into chunks that are only needed if certain features / behaviors are activated. Imagine a hypothetical component that could connect to provider A or B based on some settings -- if these libraries are big, it makes sense to delay the loading of these dependencies to when (and if!) they are needed. This need is already foreseen in the dash-table where certain features will require us to load libraries that are bigger than the table's bundle itself :) Or the more mundane case where you would have images, icon libraries or fonts bigger than a certain threshold not be included in the main chunk but rather stand beside it. That being said -- I see your point that this could be confusing to users but I don't have a good alternative either. ... 'load_dynamically' or 'dynamic_load' maybe? |
rmarren1
commented
Dec 7, 2018
Okay, thanks for the detailed explanation. I didn't know that terminology, but it makes sense to me now: 'dynamic' meaning roughly 'this resource will be dynamically loaded from the front-end when it is needed, so it is not included in the initial <script> tag load' Thanks for making those changes, looks a lot cleaner now! 💃 |
Marc-Andre-Rivet
commented
Dec 7, 2018
Since this evolved from being a small modification (.map support) to adding an entirely new feature (dynamic support) I think we should make this a minor version bump instead of a patch. |
rmarren1
commented
Dec 7, 2018
^ Agreed |
- update changelog with dynamic
Fixes#480.
Adding support for .js.map files in Dash so source map file is accessible from browser when available
Also, adding basic support for dynamic file loading. Otherwise the map gets injected as a script in the page, killing perf and just being generally useless!