feat(css-blocks): Broke up Block.ts, refactored foundational BlockObject constructs, added StateGroup concept. #71

amiller-gh · 2018-02-16T02:23:23Z

New core data structure for Blocks

Any Block may be represented by a simple tree structure, called a BlockTree. Any tree may be constructed out of two (2) types of nodes: Containers and Styles. Styles are represented by BlockClass and State objects and are the primary data we think of when reasoning about Blocks. Styles contain Ruleset data and store information about the actual CSS properties defined by the user. BlockClass and State objects are owned by the Block and StateGroup Container objects, respectively. BlockTrees are constructed using the following rules:

Blocks will always be the root node of a BlockTree;
Blocks may contain one to may BlockClasses;
BlockClasses may contain zero to may StateGroups;
StateGroups may contain one to may States;
Block and StateGroup containers always contain, at the very least, the special default Styles called the "root" class and "universal" state, respectively.

The original monolithic Block.ts file has been split up into individual files that represent each type of node in a Block tree: Block.ts, BlockClass.ts, StateGroup.ts and State.ts.

These four types of Block classes inherit from the new foundational data structure called BlockTree. Block trees define the core behavior for Container and Style nodes.

The core Container node class contains all information about a node's type, position in the tree hierarchy, and blocks they inherit from and provide methods for interacting with these relationships.

Style nodes, as mentioned above, are simply extended Container nodes that also hold information about the source CSS styles and may largely be distinguished by the presence of their RulesetContainer objects.

Because Containers and Styles are a recursive data model, they have a unique typing definition. Any node type defined for the tree – Block.ts, BlockClass.ts, StateGroup.ts and State.ts – must be provided types the Node<Self, Root, Parent, Child> interface.

To support these incoming, recursive, Block node typings for all node types, the core Container and Style definitions use the any type in some locations. But, don't be fooled! These typing holes are quickly plugged and never exposed outside the BlockTree class definitions, and are never actually exported to the larger project! In src/Block/BlockTree/index.ts we define the Source, Sink and regular Node types for both Containers and Styles. Only these classes are actually exported to the outside world, and these definitions plug the few holes in our generic, recursively typed, tree data structure. Once Block.ts, BlockClass.ts, StateGroup.ts and State.ts are exposed to these structures, they only see a fully typed tree data model to interface with.

amiller-gh · 2018-02-26T21:07:19Z

packages/css-blocks/package.json

@@ -67,6 +67,7 @@
    "css-property-parser": "^1.0.5",
    "debug": "^2.6.8",
    "inline-source-map-comment": "^1.0.5",
+    "json-parse-better-errors": "^1.0.1",


Not sure why this is here, will check it out.

yeah, I don't think this PR should include any changes to package.json or yarn.lock.

chriseppstein · 2018-02-28T01:08:48Z

packages/css-blocks/src/Block/Inheritable.ts

+   */
+  public setBase(base: Self) {
+    this._base = base;
+  }


This should only be allowed on the root. The _base property is a cache for all other types. We need to find a way to forbid this from being called either through type constraints, moving the method, or failing that a runtime check.

Moved it to Block.ts, I don't think theres a good way to keep it in inheritable.ts and restrict it just to root nodes.

chriseppstein · 2018-02-28T01:11:57Z

packages/css-blocks/src/Block/Inheritable.ts

+    if (!this._children.has(name)) {
+      this.setChild(key, this.newChild(name));
+    }
+    return this._children.get(key) as Child;


use ! to remove the undefined from the return type of the map instead of casting.

chriseppstein · 2018-02-28T01:18:40Z

packages/css-blocks/src/Block/StateGroup.ts

+    return state;
+  }
+  getState(name: string): State | null { return name ? this.getChild(name) : this.getChild(UNIVERSAL_STATE); }
+  resolveState(name: string): State | null { return name ? this.resolveChild(name) : this.resolveChild(UNIVERSAL_STATE); }


Both getState and resolveState require a name, which means that these ternaries on name are testing for empty string? I'm not a fan of using empty string as a special argument... I think passing no name is better. Also, these methods need some documentation.

👍 This was a copy pasta mistake I think.

chriseppstein · 2018-02-28T01:43:25Z

packages/css-blocks/src/query.ts

@@ -24,7 +24,8 @@ export class QueryKeySelector implements Query {
    this.impl = new QueryKeySelectorImpl(new Element(tag, attrs));
  }

-  execute(container: postcss.Container, block?: Block): ClassifiedParsedSelectors {
-    return this.impl.execute(container, block);
+  // Oooohhhh I don't like this! No part of Opticss should know about css-blocks data structures.


I agree. It takes a SelectorFactory instance which is an abstract interface defined by OptiCSS and which is implemented by the block. removing it from here will hurt performance. If it offends you too much, you can have block hold a reference to a SelectorCache instance from opticss. I see that you've made some changes to opticss so that parseSelector has it's own cache. I made comments on that PR. In general, we shouldn't force a particular caching strategy in opticss. By accepting a selector factory, the caller can control how cache eviction will work. Also, because postcss doesn't have any selector parsing ability, it's hard to know if the selector instance has been mutated intentionally and not yet sync'd back to the Rule or is stale.

Reverted the selector factory changes to Block and instead made Inheritable implement SelectorFacotry so we don't lock the getParsedSelectors method down only to Blocks. The getParsedSelectors will fetch from the root's selector cache for any given block object tree.

chriseppstein · 2018-02-28T02:09:24Z

packages/css-blocks/src/Block/BlockClass.ts

+  }
+
+  // TODO: Implement lookup relative to BlockClass.
+  public lookup(): undefined { return undefined; }


Let's file an issue to track this and remove all the lookup methods with this signature from this PR.

chriseppstein · 2018-02-28T02:10:44Z

packages/css-blocks/src/Block/BlockClass.ts

+   * @param stateName The State's name to ensure.
+   * @param subStateName  Optional state group for lookup/registration
+   */
+  ensureSubState(groupName: string, subStateName: string): State {


Is this a misnomer now?

👍 removed in favor of other implementation

…ect constructs, added StateGroup concept. This is a BREAKING CHANGES (obvs).

Fixed linting errors. Plugged typing holes in BlockTree. Air tight now!

- Disentangle Block from opticss - Remove SelectorFactory concept in favor of better caching in parseSelector (see opticss PR) - Comment all BlockTree modules - Add BlockTree tests - Start Style object / RulesetContainer tests - Begin crafting final Node.lookup interface for BlockTrees - Add optional non-silent mode to Node.lookup. If an error occurs during lookup, request an error to be thrown.

After spending considerable time exploring this new structure, I found the BlockTree abstraction mostly obfuscated the connections between core models of the Block (Block, Class, and State). By keeping the Inheritable data type and going directly to the concrete classes without an intermediate abstraction, we're still able to keep the Block types split up, but have less cognitive overhead while navigating the data model.

The source attributes for the state attribute required handling for universal states and also it was no longer including the class's source attributes.

- Inheritable now implements SelectorFactory - setBase is only available on Block objects - Remove LocalScope from project. Not needed after refactor. - Webpack loader exports as default as expected again. - Remove Block's internal OBJ_REF_SPLITTER path parser. - Remove unionInto utility from main package exports.

- New validator ensures no two states from the same state group may be applied. - Fix bug in JSX state group application to analysis. Respects inheritance now. - Add inheritance aware children getters to Inheritable. - Update webpackLoader to have a default export, as expected by Webpack.

amiller-gh force-pushed the state_containers branch from fb2b0fd to d4f9f87 Compare February 16, 2018 02:33

amiller-gh force-pushed the state_containers branch 3 times, most recently from e483eec to e34636b Compare February 26, 2018 17:51

amiller-gh commented Feb 26, 2018

View reviewed changes

chriseppstein mentioned this pull request Feb 27, 2018

State containers refactor #73

Merged

chriseppstein reviewed Feb 28, 2018

View reviewed changes

chriseppstein force-pushed the state_containers branch from ca25918 to e198abe Compare March 1, 2018 01:36

amiller-gh and others added 17 commits March 2, 2018 10:52

feat(css-blocks): Broke up Block.ts, refactored foundational BlockObj…

493a5d5

…ect constructs, added StateGroup concept. This is a BREAKING CHANGES (obvs).

feat(css-blocks): Full type safety for all BlockTree objects.

f5384ba

fix(css-blocks): Pull in new linting rules from master.

06cc47e

Fixed linting errors. Plugged typing holes in BlockTree. Air tight now!

fix(css-blocks): Rename BlockTree node types, clea up exports.

182076f

test(css-blocks): Add tests for BlockTree. Improve core data structure.

8da1de4

feat(css-blocks): Don't expose generic base class methods as public.

7cc515d

feat(css-blocks): Casting through object is sufficient.

960bf07

chore(css-blocks): Remove legacy state API.

5c2639c

chore: Unify impl of asSource for StateGroup and State.

fbcf541

chore: Fix error messages.

3c4dec9

fix: Correct the SourceAttributes returned by new State model.

1e13ba2

The source attributes for the state attribute required handling for universal states and also it was no longer including the class's source attributes.

fix: Fix broken test now that getGroups returns everything.

ea2b530

fix: Take inheritance into account during analysis.

648bff1

docs: Add API docs for methods that changed signatures.

1df3a40

chore: Remove unimplemented lookup methods.

80330e4

chriseppstein and others added 5 commits March 2, 2018 10:53

build(ci): Re-enable TravisCI.

155fa8e

build(ci): Re-enable TravisCI (take 2).

3ab30b0

build(ci): Re-enable TravisCI (take 3).

baa76a7

amiller-gh force-pushed the state_containers branch from b67f16d to 07863fe Compare March 2, 2018 18:55

chore(css-blocks): Re-name tests to match current code artifacts.

afa4cf2

amiller-gh merged commit 4e69970 into master Mar 2, 2018

amiller-gh deleted the state_containers branch March 2, 2018 20:16

This was referenced Mar 3, 2018

Further State/SubState refactoring #60

Closed

LocalScope is confusing #55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(css-blocks): Broke up Block.ts, refactored foundational BlockObject constructs, added StateGroup concept. #71

feat(css-blocks): Broke up Block.ts, refactored foundational BlockObject constructs, added StateGroup concept. #71

amiller-gh commented Feb 16, 2018 •

edited

Loading

amiller-gh Feb 26, 2018

chriseppstein Feb 28, 2018

chriseppstein Feb 28, 2018

amiller-gh Mar 1, 2018

chriseppstein Feb 28, 2018

chriseppstein Feb 28, 2018

amiller-gh Mar 1, 2018

chriseppstein Feb 28, 2018

amiller-gh Mar 1, 2018

chriseppstein Feb 28, 2018

chriseppstein Feb 28, 2018

amiller-gh Mar 1, 2018

feat(css-blocks): Broke up Block.ts, refactored foundational BlockObject constructs, added StateGroup concept. #71

feat(css-blocks): Broke up Block.ts, refactored foundational BlockObject constructs, added StateGroup concept. #71

Conversation

amiller-gh commented Feb 16, 2018 • edited Loading

New core data structure for Blocks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amiller-gh commented Feb 16, 2018 •

edited

Loading