Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add external_js/css_urls to dash init. #305

Merged
merged 6 commits into from
Aug 2, 2018
Merged

Add external_js/css_urls to dash init. #305

merged 6 commits into from
Aug 2, 2018

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Jul 26, 2018

Solution for #302.

Add two keywords arguments to Dash.__init__:

  • external_scripts_urls to include javascript files.
  • external_css_urls to include stylesheets.

They are both list of urls:

import dash

external_js = [
    'https://www.google-analytics.com/analytics.js',
    'https://cdn.polyfill.io/v2/polyfill.min.js'
]

external_css = [
    'https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css',
    'https://codepen.io/chriddyp/pen/bWLwgP.css'
]

app = dash.Dash(
    external_script_urls=external_js, 
    external_css_urls=external_css)

@chriddyp
Copy link
Member

chriddyp commented Jul 26, 2018

I personally love this. One more step towards getting rid of app.scripts.append_script and app.css.append_css.

Now, what should the name be? I guess technically it doesn't even need to be "external". And should we say "script" or "js" or "javascript"?

The permutations:

  • javascript_srcs=[...]
  • javascript_urls=[...]
  • js_srcs=[...]
  • js_urls=[...]
  • css_hrefs=[...]
  • css_urls=[...]

cc @plotly/dash as well

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Jul 26, 2018

I put external because they are always loaded externally, I added a prop to serve it from external even if serve_locally=True, otherwise they wouldn't be included with that option and we already have the assets for loading from disk so it's really just for external urls.

@rmarren1
Copy link
Contributor

I like having the word external in it to make it super clear what is intended, and I think beginners who are still googling most things and copy-pasting code would be more likely to catch an error before it occurs if they intended a local script.

dash/dash.py Outdated
((self.css.append_css, x)
for x in (external_css_urls or []))):
method({'external_url': resource, 'external_only': True})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should just add these URLs no matter what. In other words, reserve the serve_locally argument just for the component CSS where there will always be a local and a remote version. Slowly, I'd like to move ourselves away from the app.scripts.method interface as well.

Users may want to serve the assets locally because unpkg is unreliable for them or its blocked for them but load the JS or CSS urls from some other CDN. Or, they might have an internal CDN where they host assets but they can't load external assets from unpkg.

So instead, let's keep this as a private variable or something and then just embed it directly in the _generate_scirpts_html function:

dash/dash/dash.py

Lines 312 to 332 in 54ca4a3

# Dash renderer has dependencies like React which need to be rendered
# before every other script. However, the dash renderer bundle
# itself needs to be rendered after all of the component's
# scripts have rendered.
# The rest of the scripts can just be loaded after React but before
# dash renderer.
# pylint: disable=protected-access
srcs = self._collect_and_register_resources(
self.scripts._resources._filter_resources(
dash_renderer._js_dist_dependencies
) +
self.scripts.get_all_scripts() +
self.scripts._resources._filter_resources(
dash_renderer._js_dist
)
)
return '\n'.join([
'<script src="{}"></script>'.format(src)
for src in srcs
])
(or however else you want to organize it) rather than embedding it through append_script

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nevermind I see that external_only will embed it no matter what. OK, then that weakens my argument for not using append_script. I still don't really like the app.scripts... and app.css... namespace and I'd like for us to move away from it. However, I guess I don't have strong feelings about whether we should do this incrementally (i.e. not use it in this PR) or remove it all at once in a major version upgrade down the line.

@chriddyp
Copy link
Member

Alright, this looks good to me. I'm curious about your thoughts w.r.t. #305 (comment) but I won't let it block this PR. You're welcome to keep it as is or begin migrating away from append_script and add it directly to _generate_scripts_html. Up to you!

💃

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Jul 31, 2018

I'd like us to move from the css and js resources as I don't really like that system but it works.

I hadn't thought about just putting them in _generate_scripts_html and _generate_css_dist_html, I think it would work too, I'll try that.

@ngnpope
Copy link
Contributor

ngnpope commented Aug 1, 2018

I have a few comments here:

  1. What control is there over ordering? Are internal scripts/styles always output before external?
  2. Why not allow setting attributes, e.g. integrity, crossorigin, media, type, etc.
    • See my previous comment about this: allow optional header and footer #171 (comment)
    • Can we allow url strings or dicts of attributes for <link rel="stylesheet"> and <script>.
    • In this case it would make more sense to drop the _urls suffix from the new arguments.

As an aside, Dash also serves up it's internal resources from unpkg.com and, as this is an untrusted third-party, these should also use SRI. This would mean adjusting the code as in my comment mentioned above and generating hashes automatically on build of bundle.js in component bundles.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Aug 1, 2018

What control is there over ordering? Are internal scripts/styles always output before external?

External scripts/styles are output after the assets. The renderer script is always loaded last.

Why not allow setting attributes, e.g. integrity, crossorigin, media, type, etc.

I thought about it and I opted for a list of string for simplicity of usage, could put a check if it's string or a dict then format the tag accordingly.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Aug 1, 2018

I added support for tags with custom attributes, they can be mixed with simple urls. Also removed _urls prefix from the keywords.

Example:

app = dash.Dash(external_scripts=[
    'https://www.google-analytics.com/analytics.js',
    {
        'src': 'https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.10/lodash.core.js',
        'integrity': 'sha256-Qqd/EfdABZUcAxjOkMi8eGEivtdTkh3b65xCZL4qAQA=',
        'crossorigin': 'anonymous'
    }
])

With that in place, we could add SRI to the bundles resources on unpkg as @pope1ni suggested in a future PR.

@chriddyp
Copy link
Member

chriddyp commented Aug 1, 2018

Nicely done @T4rk1n , this looks great to me. Many thanks for the feedback @pope1ni !

💃

@T4rk1n T4rk1n merged commit 1f27bac into master Aug 2, 2018
Dash - A Pleasant and Productive Developer Experience automation moved this from In progress to Done Aug 2, 2018
@T4rk1n T4rk1n deleted the add-js-css-init branch August 2, 2018 15:30
@rmarren1
Copy link
Contributor

Doesn't look like this is explained here https://dash.plot.ly/external-resources

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants