diff --git a/dash/CHANGELOG.md b/dash/CHANGELOG.md index ad5d1a350c..ef8b9027a4 100644 --- a/dash/CHANGELOG.md +++ b/dash/CHANGELOG.md @@ -1,5 +1,7 @@ ## Unreleased ### Changed +- [#753](https://github.com/plotly/dash/pull/753) `Component` no longer inherits `MutableMapping`, so `values`, `keys`, and more are no longer methods. This fixed an issue reported in [dcc](https://github.com/plotly/dash-core-components/issues/440) where components with certain prop names defined but not provided would cause a failure to render. During component generation we now disallow all props with leading underscores or matching a few remaining reserved words: `UNDEFINED`, `REQUIRED`, `to_plotly_json`, `available_properties`, and `available_wildcard_properties`. + - [#739](https://github.com/plotly/dash/pull/739) Allow the Flask app to be provided to Dash after object initialization. This allows users to define Dash layouts etc when using the app factory pattern, or any other pattern that inhibits access to the app object. This broadly complies with the flask extension API, allowing Dash to be considered as a Flask extension where it needs to be. - [#722](https://github.com/plotly/dash/pull/722) Assets are served locally by default. Both JS scripts and CSS files are affected. This improves robustness and flexibility in numerous situations, but in certain cases initial loading could be slowed. To restore the previous CDN serving, set `app.scripts.config.serve_locally = False` (and similarly with `app.css`, but this is generally less important). diff --git a/dash/dash.py b/dash/dash.py index 1e31a74ffd..292d8f207b 100644 --- a/dash/dash.py +++ b/dash/dash.py @@ -914,7 +914,8 @@ def _value_is_valid(val): def _validate_value(val, index=None): # val is a Component if isinstance(val, Component): - for p, j in val.traverse_with_paths(): + # pylint: disable=protected-access + for p, j in val._traverse_with_paths(): # check each component value in the tree if not _value_is_valid(j): _raise_invalid( @@ -1194,7 +1195,8 @@ def _validate_layout(self): layout_id = getattr(self.layout, 'id', None) component_ids = {layout_id} if layout_id else set() - for component in to_validate.traverse(): + # pylint: disable=protected-access + for component in to_validate._traverse(): component_id = getattr(component, 'id', None) if component_id and component_id in component_ids: raise exceptions.DuplicateIdError( diff --git a/dash/development/base_component.py b/dash/development/base_component.py index 01f8189d3e..79e7110c0c 100644 --- a/dash/development/base_component.py +++ b/dash/development/base_component.py @@ -59,7 +59,7 @@ def _check_if_has_indexable_children(item): @six.add_metaclass(ComponentMeta) -class Component(patch_collections_abc('MutableMapping')): +class Component(object): class _UNDEFINED(object): def __repr__(self): return 'undefined' @@ -184,7 +184,7 @@ def _get_set_or_delete(self, id, operation, new_item=None): # If we were in a list, then this exception will get caught raise KeyError(id) - # Supply ABC methods for a MutableMapping: + # Magic methods for a mapping interface: # - __getitem__ # - __setitem__ # - __delitem__ @@ -208,12 +208,12 @@ def __delitem__(self, id): # pylint: disable=redefined-builtin """Delete items by ID in the tree of children.""" return self._get_set_or_delete(id, 'delete') - def traverse(self): + def _traverse(self): """Yield each item in the tree.""" - for t in self.traverse_with_paths(): + for t in self._traverse_with_paths(): yield t[1] - def traverse_with_paths(self): + def _traverse_with_paths(self): """Yield each item with its path in the tree.""" children = getattr(self, 'children', None) children_type = type(children).__name__ @@ -224,7 +224,8 @@ def traverse_with_paths(self): # children is just a component if isinstance(children, Component): yield "[*] " + children_string, children - for p, t in children.traverse_with_paths(): + # pylint: disable=protected-access + for p, t in children._traverse_with_paths(): yield "\n".join(["[*] " + children_string, p]), t # children is a list of components @@ -238,12 +239,13 @@ def traverse_with_paths(self): yield list_path, i if isinstance(i, Component): - for p, t in i.traverse_with_paths(): + # pylint: disable=protected-access + for p, t in i._traverse_with_paths(): yield "\n".join([list_path, p]), t def __iter__(self): """Yield IDs in the tree of children.""" - for t in self.traverse(): + for t in self._traverse(): if (isinstance(t, Component) and getattr(t, 'id', None) is not None): diff --git a/dash/development/component_generator.py b/dash/development/component_generator.py index 69dfafb28a..48b88c0928 100644 --- a/dash/development/component_generator.py +++ b/dash/development/component_generator.py @@ -19,6 +19,16 @@ from ._py_components_generation import generate_classes_files +reserved_words = [ + 'UNDEFINED', + 'REQUIRED', + 'to_plotly_json', + 'available_properties', + 'available_wildcard_properties', + '_.*' +] + + class _CombinedFormatter( argparse.ArgumentDefaultsHelpFormatter, argparse.RawDescriptionHelpFormatter @@ -47,9 +57,13 @@ def generate_components( extract_path = pkg_resources.resource_filename("dash", "extract-meta.js") + reserved_patterns = '|'.join('^{}$'.format(p) for p in reserved_words) + os.environ["NODE_PATH"] = "node_modules" cmd = shlex.split( - "node {} {} {}".format(extract_path, ignore, components_source), + "node {} {} {} {}".format( + extract_path, ignore, reserved_patterns, components_source + ), posix=not is_windows, ) @@ -138,8 +152,8 @@ def cli(): "-p", "--package-info-filename", default="package.json", - help="The filename of the copied `package.json` \ -to `project_shortname`", + help="The filename of the copied `package.json` " + "to `project_shortname`", ) parser.add_argument( "-i", diff --git a/dash/extract-meta.js b/dash/extract-meta.js index b667a58adc..3567161bee 100644 --- a/dash/extract-meta.js +++ b/dash/extract-meta.js @@ -4,8 +4,11 @@ const fs = require('fs'); const path = require('path'); const reactDocs = require('react-docgen'); -const componentPaths = process.argv.slice(3); +const componentPaths = process.argv.slice(4); const ignorePattern = new RegExp(process.argv[2]); +const reservedPatterns = process.argv[3].split('|').map(part => new RegExp(part)); + +let failed = false; const excludedDocProps = [ 'setProps', 'id', 'className', 'style' @@ -20,13 +23,18 @@ const metadata = Object.create(null); componentPaths.forEach(componentPath => collectMetadataRecursively(componentPath) ); -writeOut(metadata); +if (failed) { + console.error('\nextract-meta failed.\n'); +} +else { + writeOut(metadata); +} function help() { console.error('usage: '); console.error( - 'extract-meta path/to/component(s) ' + - ' [path/to/more/component(s), ...] > metadata.json' + 'extract-meta ^fileIgnorePattern ^forbidden$|^props$|^patterns$' + + ' path/to/component(s) [path/to/more/component(s) ...] > metadata.json' ); } @@ -55,6 +63,20 @@ function docstringWarning(doc) { ); } +function propError(doc) { + for(const propName in doc.props) { + reservedPatterns.forEach(reservedPattern => { + if (reservedPattern.test(propName)) { + process.stderr.write( + `\nERROR: "${propName}" matches reserved word ` + + `pattern: ${reservedPattern.toString()}\n` + ); + failed = true; + } + }); + } +} + function parseFile(filepath) { const urlpath = filepath.split(path.sep).join('/'); @@ -68,6 +90,7 @@ function parseFile(filepath) { src = fs.readFileSync(filepath); const doc = metadata[urlpath] = reactDocs.parse(src); docstringWarning(doc); + propError(doc); } catch (error) { writeError(error, filepath); } diff --git a/tests/unit/dash/development/test_base_component.py b/tests/unit/dash/development/test_base_component.py index d901189ec4..739590ccf7 100644 --- a/tests/unit/dash/development/test_base_component.py +++ b/tests/unit/dash/development/test_base_component.py @@ -1,5 +1,4 @@ from collections import OrderedDict -import collections import inspect import json import os @@ -8,8 +7,17 @@ import plotly from dash.development.base_component import Component -from dash.development._py_components_generation import generate_class_string, generate_class_file, generate_class, \ - create_docstring, prohibit_events, js_to_py_type +from dash.development.component_generator import reserved_words +from dash.development._py_components_generation import ( + generate_class_string, + generate_class_file, + generate_class, + create_docstring, + prohibit_events, + js_to_py_type +) + +_dir = os.path.dirname(os.path.abspath(__file__)) Component._prop_names = ('id', 'a', 'children', 'style', ) Component._type = 'TestComponent' @@ -85,8 +93,9 @@ def test_get_item_with_nested_children_two_branches(self): def test_get_item_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 c, c1, c2, c3, c4, c5 = nested_tree() + keys = [k for k in c] self.assertEqual( - list(c.keys()), + keys, [ '0.0', '0.1', @@ -135,7 +144,8 @@ def test_set_item_with_nested_children_with_mixed_strings_and_without_lists(self def test_del_item_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 c = nested_tree()[0] - for key in reversed(list(c.keys())): + keys = reversed([k for k in c]) + for key in keys: c[key] del c[key] with self.assertRaises(KeyError): @@ -143,7 +153,7 @@ def test_del_item_with_nested_children_with_mixed_strings_and_without_lists(self def test_traverse_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 c, c1, c2, c3, c4, c5 = nested_tree() - elements = [i for i in c.traverse()] + elements = [i for i in c._traverse()] self.assertEqual( elements, c.children + [c3] + [c2] + c2.children @@ -153,19 +163,12 @@ def test_traverse_with_tuples(self): # noqa: E501 c, c1, c2, c3, c4, c5 = nested_tree() c2.children = tuple(c2.children) c.children = tuple(c.children) - elements = [i for i in c.traverse()] + elements = [i for i in c._traverse()] self.assertEqual( elements, list(c.children) + [c3] + [c2] + list(c2.children) ) - def test_iter_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 - c = nested_tree()[0] - keys = list(c.keys()) - # get a list of ids that __iter__ provides - iter_keys = [i for i in c] - self.assertEqual(keys, iter_keys) - def test_to_plotly_json_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 c = nested_tree()[0] Component._namespace @@ -244,17 +247,6 @@ def test_get_item_raises_key_if_id_doesnt_exist(self): with self.assertRaises(KeyError): c3['0'] - def test_equality(self): - # TODO - Why is this the case? How is == being performed? - # __eq__ only needs __getitem__, __iter__, and __len__ - self.assertTrue(Component() == Component()) - self.assertTrue(Component() is not Component()) - - c1 = Component(id='1') - c2 = Component(id='2', children=[Component()]) - self.assertTrue(c1 == c2) - self.assertTrue(c1 is not c2) - def test_set_item(self): c1a = Component(id='1', children='Hello world') c2 = Component(id='2', children=c1a) @@ -450,6 +442,9 @@ def test_len(self): ])), 3) def test_iter(self): + # The mixin methods from MutableMapping were cute but probably never + # used - at least not by us. Test that they're gone + # keys, __contains__, items, values, and more are all mixin methods # that we get for free by inheriting from the MutableMapping # and behave as according to our implementation of __iter__ @@ -469,40 +464,37 @@ def test_iter(self): Component(children=[Component(id='8')]), ] ) - # test keys() - keys = [k for k in list(c.keys())] - self.assertEqual(keys, ['2', '3', '4', '5', '6', '7', '8']) - self.assertEqual([i for i in c], keys) - # test values() - components = [i for i in list(c.values())] - self.assertEqual(components, [c[k] for k in keys]) + mixins = ['clear', 'get', 'items', 'keys', 'pop', 'popitem', + 'setdefault', 'update', 'values'] + + for m in mixins: + assert not hasattr(c, m), 'should not have method ' + m + + keys = ['2', '3', '4', '5', '6', '7', '8'] - # test __iter__() for k in keys: # test __contains__() - self.assertTrue(k in c) + assert k in c, 'should find key ' + k + # test __getitem__() + assert c[k].id == k, 'key {} points to the right item'.format(k) - # test __items__ - items = [i for i in list(c.items())] - self.assertEqual(list(zip(keys, components)), items) + # test __iter__() + keys2 = [] + for k in c: + keys2.append(k) + assert k in keys, 'iteration produces key ' + k - def test_pop(self): - c2 = Component(id='2') - c = Component(id='1', children=c2) - c2_popped = c.pop('2') - self.assertTrue('2' not in c) - self.assertTrue(c2_popped is c2) + assert len(keys) == len(keys2), 'iteration produces no extra keys' class TestGenerateClassFile(unittest.TestCase): def setUp(self): - json_path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'metadata_test.json') + json_path = os.path.join(_dir, 'metadata_test.json') with open(json_path) as data_file: json_string = data_file.read() data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.data = data @@ -537,9 +529,7 @@ def setUp(self): self.written_class_string = f.read() # The expected result for both class string and class file generation - expected_string_path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'metadata_test.py' - ) + expected_string_path = os.path.join(_dir, 'metadata_test.py') with open(expected_string_path, 'r') as f: self.expected_class_string = f.read() @@ -567,12 +557,11 @@ def test_class_file(self): class TestGenerateClass(unittest.TestCase): def setUp(self): - path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'metadata_test.json') + path = os.path.join(_dir, 'metadata_test.json') with open(path) as data_file: json_string = data_file.read() data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.data = data @@ -583,14 +572,11 @@ def setUp(self): namespace='TableComponents' ) - path = os.path.join( - 'tests', 'unit', 'dash', 'development', - 'metadata_required_test.json' - ) + path = os.path.join(_dir, 'metadata_required_test.json') with open(path) as data_file: json_string = data_file.read() required_data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.required_data = required_data @@ -753,15 +739,36 @@ def test_required_props(self): with self.assertRaises(Exception): self.ComponentClassRequired(children='test') + def test_attrs_match_forbidden_props(self): + assert '_.*' in reserved_words, 'props cannot have leading underscores' + + # props are not added as attrs unless explicitly provided + # except for children, which is always set if it's a prop at all. + expected_attrs = set(reserved_words + ['children']) - set(['_.*']) + c = self.ComponentClass() + base_attrs = set(dir(c)) + extra_attrs = set(a for a in base_attrs if a[0] != '_') + + assert extra_attrs == expected_attrs, \ + 'component has only underscored and reserved word attrs' + + # setting props causes them to show up as attrs + c2 = self.ComponentClass('children', id='c2', optionalArray=[1]) + prop_attrs = set(dir(c2)) + + assert base_attrs - prop_attrs == set([]), 'no attrs were removed' + assert ( + prop_attrs - base_attrs == set(['id', 'optionalArray']) + ), 'explicit props were added as attrs' + class TestMetaDataConversions(unittest.TestCase): def setUp(self): - path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'metadata_test.json') + path = os.path.join(_dir, 'metadata_test.json') with open(path) as data_file: json_string = data_file.read() data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.data = data @@ -940,12 +947,11 @@ def assert_docstring(assertEqual, docstring): class TestFlowMetaDataConversions(unittest.TestCase): def setUp(self): - path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'flow_metadata_test.json') + path = os.path.join(_dir, 'flow_metadata_test.json') with open(path) as data_file: json_string = data_file.read() data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.data = data