update get_asset_url function

[karosc]

This pull request addresses #1527 by adding and if statement to the get_asset_url function in dash.py instructing the app to use the external asset path if it is defined.

I have tested this with an short I wrote and published. If you run the app and examine the src attribute of the image, it references and image on my own site karosc.github.io.

[alexcjohnson]

Very nice! Could be a little DRYer, as it's only the first arg to get_asset_path that needs to change, but that's not a big deal.

Can you add a couple of unit tests of app.get_asset_url? Should cover both the local and external cases, make an app and test a couple of asset urls for that app, but we don't need to run the app (so the external url need not be valid). test_configs.py looks like a good place to put it, and it would look something like these two:

dash/tests/unit/test_configs.py

Lines 147 to 179 in 3402cb8

	@pytest.mark.parametrize(
	"name, server, expected",
	[
	(None, True, "__main__"),
	("test", True, "test"),
	("test", False, "test"),
	(None, Flask("test"), "test"),
	("test", Flask("other"), "test"),
	],
	)
	def test_app_name_server(empty_environ, name, server, expected):
	app = Dash(name=name, server=server)
	assert app.config.name == expected
	@pytest.mark.parametrize(
	"prefix, partial_path, expected",
	[
	("/", "", "/"),
	("/my-dash-app/", "", "/my-dash-app/"),
	("/", "/", "/"),
	("/my-dash-app/", "/", "/my-dash-app/"),
	("/", "/page-1", "/page-1"),
	("/my-dash-app/", "/page-1", "/my-dash-app/page-1"),
	("/", "/page-1/", "/page-1/"),
	("/my-dash-app/", "/page-1/", "/my-dash-app/page-1/"),
	("/", "/page-1/sub-page-1", "/page-1/sub-page-1"),
	("/my-dash-app/", "/page-1/sub-page-1", "/my-dash-app/page-1/sub-page-1"),
	],
	)
	def test_pathname_prefix_relative_url(prefix, partial_path, expected):
	path = get_relative_path(prefix, partial_path)
	assert path == expected

After that, we'll just need a changelog entry and it'll be ready to go!

[karosc]

does this seem more DRY? Not sure exactly what you were referring to.

def get_asset_url(self, path):
        if self.config.assets_external_path:
            prefix = self.config.assets_external_path
        else:
	    prefix = self.config.requests_pathname_prefix

        asset = get_asset_path(
                     prefix,
                     path,
                     self.config.assets_url_path.lstrip("/"),
               )

        return asset

[alexcjohnson]

Yep, that's about what I had in mind. But again, not a big deal.

[alexcjohnson]

Hmm, maybe we could just call get_asset_url here?

[karosc]

That works! Passing all tests.

[alexcjohnson]

Looking good, just a couple of minor comments above, but also I think it needs linting (I sympathize with wanting aligned values in the test matrix but I don't think black will go for it 😅 ) - Probably just needs npm run format.

[karosc]

running npm format changed a bunch of other files, sorry for creating more review work...🤷‍♂️

[alexcjohnson]

Ah, I'm guessing you're on a newer version of black. What version do you have installed? We've talked about the need to update to 20.8b1 (and maybe now is the right time to do that across Dash, cc @Marc-Andre-Rivet?) but currently we have 19.10b0

dash/requires-dev.txt

Line 11 in c5ba38f

black==19.10b0;python_version>="3.0"

[karosc]

oh shoot, you're right I am running 20.8b1. I can downgrade and reformat then if that's when you want to do.

[alexcjohnson]

Downgrading and reformatting would be the easiest way to get this merged quickly 😄 We may upgrade but that's going to take a little work because we'll want to do it evenly across all the dash repos.

[karosc]

Alright, I reset to before I ran the format routine, downgraded black to 19.10b0 and then redid the format. Things came out to be more expected since only my changes were reformatted by black.

[alexcjohnson]

Very nice - thank you! 💃