From e76055c829417593308b4ae933afeb34e6262c75 Mon Sep 17 00:00:00 2001 From: rmorshea Date: Thu, 13 Jan 2022 23:10:03 -0800 Subject: [PATCH 1/4] clean up web module from file and template logic --- .../_examples/super_simple_chart/app.py | 8 +- src/idom/web/module.py | 92 ++++++++++++------- tests/test_web/test_module.py | 26 +++++- 3 files changed, 82 insertions(+), 44 deletions(-) diff --git a/docs/source/escape-hatches/_examples/super_simple_chart/app.py b/docs/source/escape-hatches/_examples/super_simple_chart/app.py index 9f0563a97..3884315be 100644 --- a/docs/source/escape-hatches/_examples/super_simple_chart/app.py +++ b/docs/source/escape-hatches/_examples/super_simple_chart/app.py @@ -4,13 +4,7 @@ file = Path(__file__).parent / "super-simple-chart.js" -ssc = web.module_from_file( - "super-simple-chart", - file, - fallback="⌛", - # normally this option is not required - replace_existing=True, -) +ssc = web.module_from_file("super-simple-chart", file, fallback="⌛", symlink=True) SuperSimpleChart = web.export(ssc, "SuperSimpleChart") diff --git a/src/idom/web/module.py b/src/idom/web/module.py index 67a4acec1..31d790d7f 100644 --- a/src/idom/web/module.py +++ b/src/idom/web/module.py @@ -1,10 +1,12 @@ from __future__ import annotations +import filecmp import shutil -from dataclasses import dataclass +from dataclasses import dataclass, replace from functools import partial from pathlib import Path from string import Template +from tempfile import NamedTemporaryFile from typing import Any, List, NewType, Optional, Set, Tuple, Union, overload from urllib.parse import urlparse @@ -74,6 +76,9 @@ def module_from_url( ) +_FROM_TEMPLATE_DIR = "__from_template__" + + def module_from_template( template: str, package: str, @@ -83,6 +88,7 @@ def module_from_template( resolve_exports: bool = IDOM_DEBUG_MODE.current, resolve_exports_depth: int = 5, unmount_before_update: bool = False, + replace_existing: bool = False, ) -> WebModule: """Create a :class:`WebModule` from a framework template @@ -121,6 +127,9 @@ def module_from_template( only be used if the imported package failes to re-render when props change. Using this option has negative performance consequences since all DOM elements must be changed on each render. See :issue:`461` for more info. + replace_existing: + Whether to replace the source for a module with the same name if it already + exists and has different content. Otherwise raise an error. """ # We do this since the package may be any valid URL path. Thus we may need to strip # object parameters or query information so we save the resulting template under the @@ -140,27 +149,30 @@ def module_from_template( if not template_file.exists(): raise ValueError(f"No template for {template_file_name!r} exists") - target_file = _web_module_path(package_name, "from-template") - if not target_file.exists(): - target_file.parent.mkdir(parents=True, exist_ok=True) - target_file.write_text( - Template(template_file.read_text()).substitute( - {"PACKAGE": package, "CDN": cdn} - ) + variables = {"PACKAGE": package, "CDN": cdn} + content = Template(template_file.read_text()).substitute(variables) + + with NamedTemporaryFile(mode="r+") as file: + file.write(content) + file.seek(0) # set the cursor back to begining of file + + module = module_from_file( + ( + _FROM_TEMPLATE_DIR + + "/" + + package_name + + module_name_suffix(package_name) + ), + file.name, + fallback, + resolve_exports, + resolve_exports_depth, + symlink=False, + unmount_before_update=unmount_before_update, + replace_existing=replace_existing, ) - return WebModule( - source="from-template/" + package_name + module_name_suffix(package_name), - source_type=NAME_SOURCE, - default_fallback=fallback, - file=target_file, - export_names=( - resolve_module_exports_from_file(target_file, resolve_exports_depth) - if resolve_exports - else None - ), - unmount_before_update=unmount_before_update, - ) + return replace(module, file=None) def module_from_file( @@ -196,23 +208,29 @@ def module_from_file( elements must be changed on each render. See :issue:`461` for more info. replace_existing: Whether to replace the source for a module with the same name if it already - exists. Otherwise raise an error. + exists and has different content. Otherwise raise an error. """ source_file = Path(file) target_file = _web_module_path(name) if not source_file.exists(): raise FileNotFoundError(f"Source file does not exist: {source_file}") - elif target_file.exists() or target_file.is_symlink(): - if not replace_existing: - raise FileExistsError(f"{name!r} already exists as {target_file.resolve()}") - else: - target_file.unlink() - target_file.parent.mkdir(parents=True, exist_ok=True) - if symlink: - target_file.symlink_to(source_file) - else: - shutil.copy(source_file, target_file) + if not target_file.exists(): + _copy_file(target_file, source_file, symlink) + elif not ( + symlink + and target_file.is_symlink() + and target_file.resolve() == source_file.resolve() + ): + if replace_existing: + target_file.unlink() + _copy_file(target_file, source_file, symlink) + elif not filecmp.cmp( + str(source_file.resolve()), + str(target_file.resolve()), + shallow=False, + ): + raise FileExistsError(f"{name!r} already exists as {target_file.resolve()}") return WebModule( source=name + module_name_suffix(name), @@ -228,6 +246,14 @@ def module_from_file( ) +def _copy_file(target: Path, source: Path, symlink: bool) -> None: + target.parent.mkdir(parents=True, exist_ok=True) + if symlink: + target.symlink_to(source) + else: + shutil.copy(source, target) + + class _VdomDictConstructor(Protocol): def __call__( self, @@ -326,10 +352,8 @@ def _make_export( ) -def _web_module_path(name: str, prefix: str = "") -> Path: +def _web_module_path(name: str) -> Path: name += module_name_suffix(name) directory = IDOM_WED_MODULES_DIR.current - if prefix: - directory /= prefix path = directory.joinpath(*name.split("/")) return path.with_suffix(path.suffix) diff --git a/tests/test_web/test_module.py b/tests/test_web/test_module.py index 15067b896..958616ab3 100644 --- a/tests/test_web/test_module.py +++ b/tests/test_web/test_module.py @@ -126,8 +126,14 @@ def test_module_from_file_source_conflict(tmp_path): second_file = tmp_path / "second.js" second_file.touch() + # ok, same content + idom.web.module_from_file("temp", second_file) + + third_file = tmp_path / "third.js" + third_file.write_text("something-different") + with pytest.raises(FileExistsError, match="already exists"): - idom.web.module_from_file("temp", second_file) + idom.web.module_from_file("temp", third_file) def test_web_module_from_file_symlink(tmp_path): @@ -143,6 +149,20 @@ def test_web_module_from_file_symlink(tmp_path): assert module.file.resolve().read_text() == "hello world!" +def test_web_module_from_file_symlink_twice(tmp_path): + file_1 = tmp_path / "temp_1.js" + file_1.touch() + + idom.web.module_from_file("temp", file_1, symlink=True) + idom.web.module_from_file("temp", file_1, symlink=True) + + file_2 = tmp_path / "temp_2.js" + file_2.write_text("something") + + with pytest.raises(FileExistsError, match="already exists"): + idom.web.module_from_file("temp", file_2, symlink=True) + + def test_web_module_from_file_replace_existing(tmp_path): file1 = tmp_path / "temp1.js" file1.touch() @@ -150,7 +170,7 @@ def test_web_module_from_file_replace_existing(tmp_path): idom.web.module_from_file("temp", file1) file2 = tmp_path / "temp2.js" - file2.touch() + file2.write_text("something") with pytest.raises(FileExistsError, match="already exists"): idom.web.module_from_file("temp", file2) @@ -200,7 +220,7 @@ def test_imported_components_can_render_children(driver, display): ) parent = driver.find_element("id", "the-parent") - children = parent.find_elements_by_tag_name("li") + children = parent.find_elements("tag name", "li") assert len(children) == 3 From 83d2d3f52756e7c713e5a73a4e4c8f130de7c725 Mon Sep 17 00:00:00 2001 From: rmorshea Date: Tue, 18 Jan 2022 18:06:31 -0800 Subject: [PATCH 2/4] log module replacements instead of error --- .../_examples/super_simple_chart/app.py | 2 +- src/idom/web/__init__.py | 9 +- src/idom/web/module.py | 129 +++++++++++------- tests/test_web/test_module.py | 20 ++- 4 files changed, 101 insertions(+), 59 deletions(-) diff --git a/docs/source/escape-hatches/_examples/super_simple_chart/app.py b/docs/source/escape-hatches/_examples/super_simple_chart/app.py index 3884315be..2f2e17556 100644 --- a/docs/source/escape-hatches/_examples/super_simple_chart/app.py +++ b/docs/source/escape-hatches/_examples/super_simple_chart/app.py @@ -4,7 +4,7 @@ file = Path(__file__).parent / "super-simple-chart.js" -ssc = web.module_from_file("super-simple-chart", file, fallback="⌛", symlink=True) +ssc = web.module_from_file("super-simple-chart", file, fallback="⌛") SuperSimpleChart = web.export(ssc, "SuperSimpleChart") diff --git a/src/idom/web/__init__.py b/src/idom/web/__init__.py index d3187366c..6fe239ed9 100644 --- a/src/idom/web/__init__.py +++ b/src/idom/web/__init__.py @@ -1,8 +1,15 @@ -from .module import export, module_from_file, module_from_template, module_from_url +from .module import ( + export, + module_from_file, + module_from_string, + module_from_template, + module_from_url, +) __all__ = [ "module_from_file", + "module_from_string", "module_from_template", "module_from_url", "export", diff --git a/src/idom/web/module.py b/src/idom/web/module.py index 31d790d7f..7f4d5f320 100644 --- a/src/idom/web/module.py +++ b/src/idom/web/module.py @@ -1,12 +1,11 @@ from __future__ import annotations -import filecmp +import logging import shutil -from dataclasses import dataclass, replace +from dataclasses import dataclass from functools import partial from pathlib import Path from string import Template -from tempfile import NamedTemporaryFile from typing import Any, List, NewType, Optional, Set, Tuple, Union, overload from urllib.parse import urlparse @@ -28,6 +27,8 @@ ) +logger = logging.getLogger(__name__) + SourceType = NewType("SourceType", str) NAME_SOURCE = SourceType("NAME") @@ -88,7 +89,6 @@ def module_from_template( resolve_exports: bool = IDOM_DEBUG_MODE.current, resolve_exports_depth: int = 5, unmount_before_update: bool = False, - replace_existing: bool = False, ) -> WebModule: """Create a :class:`WebModule` from a framework template @@ -127,9 +127,6 @@ def module_from_template( only be used if the imported package failes to re-render when props change. Using this option has negative performance consequences since all DOM elements must be changed on each render. See :issue:`461` for more info. - replace_existing: - Whether to replace the source for a module with the same name if it already - exists and has different content. Otherwise raise an error. """ # We do this since the package may be any valid URL path. Thus we may need to strip # object parameters or query information so we save the resulting template under the @@ -152,27 +149,14 @@ def module_from_template( variables = {"PACKAGE": package, "CDN": cdn} content = Template(template_file.read_text()).substitute(variables) - with NamedTemporaryFile(mode="r+") as file: - file.write(content) - file.seek(0) # set the cursor back to begining of file - - module = module_from_file( - ( - _FROM_TEMPLATE_DIR - + "/" - + package_name - + module_name_suffix(package_name) - ), - file.name, - fallback, - resolve_exports, - resolve_exports_depth, - symlink=False, - unmount_before_update=unmount_before_update, - replace_existing=replace_existing, - ) - - return replace(module, file=None) + return module_from_string( + _FROM_TEMPLATE_DIR + "/" + package_name + module_name_suffix(package_name), + content, + fallback, + resolve_exports, + resolve_exports_depth, + unmount_before_update=unmount_before_update, + ) def module_from_file( @@ -181,20 +165,16 @@ def module_from_file( fallback: Optional[Any] = None, resolve_exports: bool = IDOM_DEBUG_MODE.current, resolve_exports_depth: int = 5, - symlink: bool = False, unmount_before_update: bool = False, - replace_existing: bool = False, + symlink: bool = False, ) -> WebModule: - """Load a :class:`WebModule` from a :data:`URL_SOURCE` using a known framework + """Load a :class:`WebModule` from a given ``file`` Parameters: - template: - The name of the template to use with the given ``package`` - package: - The name of a package to load. May include a file extension (defaults to - ``.js`` if not given) - cdn: - Where the package should be loaded from. The CDN must distribute ESM modules + name: + The name of the package + file: + The file from which the content of the web module will be created. fallback: What to temporarilly display while the module is being loaded. resolve_imports: @@ -206,9 +186,8 @@ def module_from_file( only be used if the imported package failes to re-render when props change. Using this option has negative performance consequences since all DOM elements must be changed on each render. See :issue:`461` for more info. - replace_existing: - Whether to replace the source for a module with the same name if it already - exists and has different content. Otherwise raise an error. + symlink: + Whether the web module should be saved as a symlink to the given ``file``. """ source_file = Path(file) target_file = _web_module_path(name) @@ -222,15 +201,12 @@ def module_from_file( and target_file.is_symlink() and target_file.resolve() == source_file.resolve() ): - if replace_existing: - target_file.unlink() - _copy_file(target_file, source_file, symlink) - elif not filecmp.cmp( - str(source_file.resolve()), - str(target_file.resolve()), - shallow=False, - ): - raise FileExistsError(f"{name!r} already exists as {target_file.resolve()}") + logger.info( + f"Existing web module {name!r} will " + f"be replaced with {target_file.resolve()}" + ) + target_file.unlink() + _copy_file(target_file, source_file, symlink) return WebModule( source=name + module_name_suffix(name), @@ -254,6 +230,59 @@ def _copy_file(target: Path, source: Path, symlink: bool) -> None: shutil.copy(source, target) +def module_from_string( + name: str, + content: str, + fallback: Optional[Any] = None, + resolve_exports: bool = IDOM_DEBUG_MODE.current, + resolve_exports_depth: int = 5, + unmount_before_update: bool = False, +): + """Load a :class:`WebModule` whose ``content`` comes from a string. + + Parameters: + name: + The name of the package + content: + The contents of the web module + fallback: + What to temporarilly display while the module is being loaded. + resolve_imports: + Whether to try and find all the named exports of this module. + resolve_exports_depth: + How deeply to search for those exports. + unmount_before_update: + Cause the component to be unmounted before each update. This option should + only be used if the imported package failes to re-render when props change. + Using this option has negative performance consequences since all DOM + elements must be changed on each render. See :issue:`461` for more info. + """ + target_file = _web_module_path(name) + + if target_file.exists(): + logger.info( + f"Existing web module {name!r} will " + f"be replaced with {target_file.resolve()}" + ) + target_file.unlink() + + target_file.parent.mkdir(parents=True, exist_ok=True) + target_file.write_text(content) + + return WebModule( + source=name + module_name_suffix(name), + source_type=NAME_SOURCE, + default_fallback=fallback, + file=target_file, + export_names=( + resolve_module_exports_from_file(target_file, resolve_exports_depth) + if resolve_exports + else None + ), + unmount_before_update=unmount_before_update, + ) + + class _VdomDictConstructor(Protocol): def __call__( self, diff --git a/tests/test_web/test_module.py b/tests/test_web/test_module.py index 958616ab3..a9290d5a1 100644 --- a/tests/test_web/test_module.py +++ b/tests/test_web/test_module.py @@ -8,7 +8,7 @@ import idom from idom.server.sanic import PerClientStateServer -from idom.testing import ServerMountPoint +from idom.testing import ServerMountPoint, assert_idom_did_not_log, assert_idom_logged from idom.web.module import NAME_SOURCE, WebModule @@ -132,7 +132,7 @@ def test_module_from_file_source_conflict(tmp_path): third_file = tmp_path / "third.js" third_file.write_text("something-different") - with pytest.raises(FileExistsError, match="already exists"): + with assert_idom_logged(r"Existing web module .* will be replaced with"): idom.web.module_from_file("temp", third_file) @@ -154,12 +154,14 @@ def test_web_module_from_file_symlink_twice(tmp_path): file_1.touch() idom.web.module_from_file("temp", file_1, symlink=True) - idom.web.module_from_file("temp", file_1, symlink=True) + + with assert_idom_did_not_log(r"Existing web module .* will be replaced with"): + idom.web.module_from_file("temp", file_1, symlink=True) file_2 = tmp_path / "temp_2.js" file_2.write_text("something") - with pytest.raises(FileExistsError, match="already exists"): + with assert_idom_logged(r"Existing web module .* will be replaced with"): idom.web.module_from_file("temp", file_2, symlink=True) @@ -172,11 +174,9 @@ def test_web_module_from_file_replace_existing(tmp_path): file2 = tmp_path / "temp2.js" file2.write_text("something") - with pytest.raises(FileExistsError, match="already exists"): + with assert_idom_logged(r"Existing web module .* will be replaced with"): idom.web.module_from_file("temp", file2) - idom.web.module_from_file("temp", file2, replace_existing=True) - def test_module_missing_exports(): module = WebModule("test", NAME_SOURCE, None, {"a", "b", "c"}, None, False) @@ -226,3 +226,9 @@ def test_imported_components_can_render_children(driver, display): for index, child in enumerate(children): assert child.get_attribute("id") == f"child-{index + 1}" + + +def test_module_from_string(): + idom.web.module_from_string("temp", "") + with assert_idom_logged(r"Existing web module .* will be replaced with"): + idom.web.module_from_string("temp", "") From 6100ed4560f39a9c0ea777f1a051d827baa8b9d2 Mon Sep 17 00:00:00 2001 From: rmorshea Date: Tue, 18 Jan 2022 18:29:07 -0800 Subject: [PATCH 3/4] only change files if content is changed --- src/idom/web/module.py | 19 ++++++++++++------- tests/test_web/test_module.py | 4 ++-- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/idom/web/module.py b/src/idom/web/module.py index 7f4d5f320..7a6db7dfe 100644 --- a/src/idom/web/module.py +++ b/src/idom/web/module.py @@ -1,5 +1,6 @@ from __future__ import annotations +import filecmp import logging import shutil from dataclasses import dataclass @@ -189,18 +190,14 @@ def module_from_file( symlink: Whether the web module should be saved as a symlink to the given ``file``. """ - source_file = Path(file) + source_file = Path(file).resolve() target_file = _web_module_path(name) if not source_file.exists(): raise FileNotFoundError(f"Source file does not exist: {source_file}") if not target_file.exists(): _copy_file(target_file, source_file, symlink) - elif not ( - symlink - and target_file.is_symlink() - and target_file.resolve() == source_file.resolve() - ): + elif not _equal_files(source_file, target_file): logger.info( f"Existing web module {name!r} will " f"be replaced with {target_file.resolve()}" @@ -222,6 +219,14 @@ def module_from_file( ) +def _equal_files(f1: Path, f2: Path) -> bool: + f1 = f1.resolve() + f2 = f2.resolve() + return ( + (f1.is_symlink() or f2.is_symlink()) and (f1.resolve() == f2.resolve()) + ) or filecmp.cmp(str(f1), str(f2), shallow=False) + + def _copy_file(target: Path, source: Path, symlink: bool) -> None: target.parent.mkdir(parents=True, exist_ok=True) if symlink: @@ -259,7 +264,7 @@ def module_from_string( """ target_file = _web_module_path(name) - if target_file.exists(): + if target_file.exists() and target_file.read_text() != content: logger.info( f"Existing web module {name!r} will " f"be replaced with {target_file.resolve()}" diff --git a/tests/test_web/test_module.py b/tests/test_web/test_module.py index a9290d5a1..1e31f5d0a 100644 --- a/tests/test_web/test_module.py +++ b/tests/test_web/test_module.py @@ -229,6 +229,6 @@ def test_imported_components_can_render_children(driver, display): def test_module_from_string(): - idom.web.module_from_string("temp", "") + idom.web.module_from_string("temp", "old") with assert_idom_logged(r"Existing web module .* will be replaced with"): - idom.web.module_from_string("temp", "") + idom.web.module_from_string("temp", "new") From 40bfb1f284c624cd51c1c49037eff46d4d51d3b9 Mon Sep 17 00:00:00 2001 From: rmorshea Date: Tue, 18 Jan 2022 18:41:24 -0800 Subject: [PATCH 4/4] fix type annotation --- src/idom/web/module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/idom/web/module.py b/src/idom/web/module.py index 7a6db7dfe..9002ded39 100644 --- a/src/idom/web/module.py +++ b/src/idom/web/module.py @@ -242,7 +242,7 @@ def module_from_string( resolve_exports: bool = IDOM_DEBUG_MODE.current, resolve_exports_depth: int = 5, unmount_before_update: bool = False, -): +) -> WebModule: """Load a :class:`WebModule` whose ``content`` comes from a string. Parameters: