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

feat: DesignToken refactor for FF3 #6171

Merged
merged 97 commits into from
Aug 9, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
97 commits
Select commit Hold shift + click to select a range
7128199
adding interfaces for new API structure
nicholasrice May 27, 2022
9451732
adding tests for appendChild / removeChild
nicholasrice May 27, 2022
469b1d6
add a stub implementation of DesignToken
nicholasrice May 27, 2022
6452e01
adding minimal design token interface
nicholasrice May 31, 2022
c5d27e9
save-progress
nicholasrice Jun 1, 2022
5d7e8cb
adding notification logic
nicholasrice Jun 2, 2022
18f6ba2
re-organize notification propagation logic
nicholasrice Jun 2, 2022
0dba736
adding getTokenValue and tests
nicholasrice Jun 2, 2022
56eb369
add notification behavior to appndChild
nicholasrice Jun 2, 2022
1a3181b
refactor to use string id
nicholasrice Jun 2, 2022
65f70bd
rename method to retrieve all tokens for a node
nicholasrice Jun 2, 2022
afc9467
refactor to fix chai bug and issues uncovered when avoiding chai issue
nicholasrice Jun 3, 2022
dcfa22f
refactor a bit to centralize notifier for tokens on the token object …
nicholasrice Jun 6, 2022
5450c15
adding new test for no notification when assigned the same value
nicholasrice Jun 6, 2022
64a4988
fixing test
nicholasrice Jun 6, 2022
43ea386
adding subscription test
nicholasrice Jun 6, 2022
610a61e
implement isAssigned
nicholasrice Jun 6, 2022
7348d91
progress on derived tokens
nicholasrice Jun 7, 2022
f83ad12
implement mechanism to notify of static token changes
nicholasrice Jun 9, 2022
6000841
make derived token assignments notify down the token tree
nicholasrice Jun 9, 2022
7d072ab
adding and adapting test-cases from original DesignToken implementation
nicholasrice Jun 23, 2022
fc127d9
re-organization of DesignTokenNode
nicholasrice Jun 24, 2022
4972675
adding observable map to notify for derived values
nicholasrice Jun 27, 2022
8bb74ac
naieve implementation of reflecting upstream derived tokens
nicholasrice Jun 27, 2022
702946c
re-oragnize feature and add exports to facilitate testing
nicholasrice Jun 28, 2022
2c7f237
implement circular reference resolution and made a few perf changes
nicholasrice Jun 28, 2022
c23d40f
adding another circular reference test
nicholasrice Jun 28, 2022
5a08370
adding deleteTokenValue API and privatizing isDerivedFor
nicholasrice Jun 28, 2022
8050deb
adding subscription tests
nicholasrice Jun 28, 2022
a9d431e
update test names
nicholasrice Jun 29, 2022
68b801d
further test clean up
nicholasrice Jun 29, 2022
c2d7f93
more tests and slight performance improvement
nicholasrice Jun 29, 2022
0baa844
method organization
nicholasrice Jun 29, 2022
06050e6
remove observableMap implementation to provide better control over to…
nicholasrice Jun 30, 2022
acd0014
implement more specific notification behavior
nicholasrice Jul 5, 2022
e6b0e35
fixing test so it passes w/ new notification behavior
nicholasrice Jul 5, 2022
800c569
adding test package
nicholasrice Jul 5, 2022
b325bcb
adds behavior to handle observable value updates
nicholasrice Jul 7, 2022
014cac2
adding additional observable tests
nicholasrice Jul 7, 2022
967371b
refactor token notification to contain mutation type data
nicholasrice Jul 8, 2022
4aeb734
refactor notifications and fix token deletion tests
nicholasrice Jul 8, 2022
3b90057
adding notification behavior to appending a node
nicholasrice Jul 11, 2022
9282a21
update readme with perf improvement notes
nicholasrice Jul 11, 2022
19b3ed6
in-progress changes for node notification on removal of token and ref…
nicholasrice Jul 12, 2022
cb9b7aa
refactor notification behavior and fix all test
nicholasrice Jul 12, 2022
18859c1
batch notifications so the tree is reconciled before subscribers are …
nicholasrice Jul 12, 2022
2529b25
implement DesignToken over top of DesignTokenNode, CSS reflection
nicholasrice Jul 14, 2022
fab4d24
isolate design token core from FAST design token implementation
nicholasrice Jul 14, 2022
2ac114a
implement root registration
nicholasrice Jul 14, 2022
647d172
Ensure nodes for detached elements can access the defualt node
nicholasrice Jul 14, 2022
33d4d80
passing all tests
nicholasrice Jul 15, 2022
5c139d2
fixing tests
nicholasrice Jul 15, 2022
9ae5283
fixing import
nicholasrice Jul 15, 2022
7a59750
a bit of cleanup
nicholasrice Jul 15, 2022
08357b6
refactor type constraints that opens up exporting the token classes i…
nicholasrice Jul 18, 2022
ddb6cde
re-organizing files
nicholasrice Jul 18, 2022
6288009
re-organize assets, exports, and files. delete legacy design-token
nicholasrice Jul 18, 2022
2afabb3
fix bug in recursive reconciliation of tokens
nicholasrice Jul 19, 2022
d4a5357
update adaptive UI package
nicholasrice Jul 19, 2022
872dc78
consolidate notification queue
nicholasrice Jul 20, 2022
b6c2f56
add children notifier function
nicholasrice Jul 20, 2022
723cd3b
make CSSDesignToken a real CSSDirective
nicholasrice Jul 21, 2022
84a17d4
fixing circular dependency notification bug
nicholasrice Jul 21, 2022
1ccd633
fixing bug in CSS design token css value calculation
nicholasrice Jul 21, 2022
83e22d8
implement observable derived value structure
nicholasrice Jul 21, 2022
0eac178
refactor to not itterate through all derived values during update
nicholasrice Jul 25, 2022
b5e8e9c
improving performance of derived tokens
nicholasrice Jul 26, 2022
8dce1b9
fixing example
nicholasrice Jul 26, 2022
5d51044
removing un-necessary method
nicholasrice Jul 26, 2022
d93b56f
adding css directive test
nicholasrice Jul 26, 2022
8e4dd3c
create tokens with unique name
nicholasrice Jul 26, 2022
9faadf3
lazily create subscriber set for tokens
nicholasrice Jul 27, 2022
959cfa5
implement resolution strategy
nicholasrice Jul 27, 2022
d037ca3
removing debugging spies
nicholasrice Jul 27, 2022
72b40a3
fixing bug in observable bindings where the watcher was not reset if …
nicholasrice Jul 29, 2022
9baeaf6
fixing memory leak caused by observable subscribers
nicholasrice Aug 3, 2022
e32a146
cleaning up design-token-node tests
nicholasrice Aug 3, 2022
a85c0a2
removing tests for scenario that is no longer supported
nicholasrice Aug 3, 2022
9e29f8b
adding method comments
nicholasrice Aug 3, 2022
fef9ec8
remove test package
nicholasrice Aug 3, 2022
f9043a1
fixing after rebase
nicholasrice Aug 3, 2022
dca161c
Merge branch 'master' into users/nirice/design-token-refactor-explora…
nicholasrice Aug 3, 2022
81da3c7
Change files
nicholasrice Aug 3, 2022
6b646dc
Merge branch 'users/nirice/design-token-refactor-exploration' of http…
nicholasrice Aug 3, 2022
f8dc19d
Update change/@microsoft-adaptive-ui-881bac8a-4800-4331-9138-0c658998…
nicholasrice Aug 3, 2022
a9d2d15
Update change/@microsoft-fast-element-2a4431cf-f1de-44ed-bae4-392f10b…
nicholasrice Aug 3, 2022
edeb884
Update change/@microsoft-fast-foundation-d9cdf375-e6b7-4a6e-87e6-81dc…
nicholasrice Aug 3, 2022
baea9b5
removing test site from workspaces
nicholasrice Aug 3, 2022
63ea814
removing readme that is getting processesd by custom element manifest…
nicholasrice Aug 3, 2022
e1ecf34
adding API documentation
nicholasrice Aug 3, 2022
4a9a81a
removing notes file
nicholasrice Aug 3, 2022
a2f6319
fixing build
nicholasrice Aug 3, 2022
9d0adfe
Merge branch 'master' into users/nirice/design-token-refactor-explora…
nicholasrice Aug 5, 2022
7e6454b
Merge branch 'master' into users/nirice/design-token-refactor-explora…
nicholasrice Aug 8, 2022
19ecfd3
Update change/@microsoft-fast-foundation-d9cdf375-e6b7-4a6e-87e6-81dc…
nicholasrice Aug 9, 2022
fb343cf
Update packages/web-components/fast-foundation/src/design-token/fast-…
nicholasrice Aug 9, 2022
9bd20b4
Merge branch 'master' into users/nirice/design-token-refactor-explora…
nicholasrice Aug 9, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor to not itterate through all derived values during update
  • Loading branch information
nicholasrice committed Aug 3, 2022
commit 0eac178419f919170d599bc169d7d120887ea4b9
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ describe("DesignTokenNode", () => {
});
});
describe("setting a token to a derived value", () => {
it("should resolve a value for for the token being assigned from the parent node", () => {
it("should resolve a value for the token being assigned from the parent node", () => {
const token = new DesignToken<number>();
const parent = createNode();
const child = createNode(parent);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
BindingObserver,
Disposable,
observable,
Observable,
Subscriber,
} from "@microsoft/fast-element";
Expand Down Expand Up @@ -36,7 +35,8 @@ export type DesignTokenValue<T> = StaticDesignTokenValue<T> | DerivedDesignToken

class DerivedValueEvaluator<T> {
private readonly binding: BindingObserver;
public readonly dependencies = new ObservableSet<DesignToken<any>>();
private notifier = Observable.getNotifier(this);
public readonly dependencies = new Set<DesignToken<any>>();
private static cache = new WeakMap<
DerivedDesignTokenValue<any>,
DerivedValueEvaluator<any>
Expand Down Expand Up @@ -81,67 +81,27 @@ class DerivedValueEvaluator<T> {
}

public handleChange() {
Observable.getNotifier(this).notify(undefined);
}
}

/**
* Simple observable set implementation for observing additions;
*/
class ObservableSet<T> extends Set<T> {
add(value: T) {
const notify = !this.has(value);
super.add(value);

if (notify) {
Observable.getNotifier(this).notify(value);
}

return this;
this.notifier.notify(undefined);
}
}

class DerivedValue<T> implements Disposable {
@observable value: T;
value: T;
constructor(
public readonly token: DesignToken<T>,
public readonly evaluator: DerivedValueEvaluator<T>,
public readonly node: DesignTokenNode
public readonly node: DesignTokenNode,
private subscriber?: Subscriber
) {
Observable.getNotifier(evaluator.dependencies).subscribe(
this.dependencySubscriber
);
this.value = evaluator.evaluate(node, token);

const nodeNotifier = Observable.getNotifier(this.node);
for (const dependency of this.evaluator.dependencies) {
nodeNotifier.subscribe(this, dependency);
if (this.subscriber) {
Observable.getNotifier(this.evaluator).subscribe(this.subscriber);
}
}

/**
* Invoked when the dependency set changes. Used to subscribe
*/
private dependencySubscriber: Subscriber = {
handleChange: (
source: ObservableSet<DesignToken<any>>,
value: DesignToken<any>
) => {
const notifier = Observable.getNotifier(this.node);
const action = this.evaluator.dependencies.has(value)
? notifier.subscribe
: notifier.unsubscribe;
action.call(notifier, this, value);
},
};

public dispose(): void {
Observable.getNotifier(this.evaluator.dependencies).unsubscribe(
this.dependencySubscriber
);
const nodeNotifier = Observable.getNotifier(this.node);
for (const dependency of this.evaluator.dependencies) {
nodeNotifier.unsubscribe(this, dependency);
if (this.subscriber) {
Observable.getNotifier(this.evaluator).unsubscribe(this.subscriber);
}
}

Expand Down Expand Up @@ -196,6 +156,7 @@ export class DesignTokenNode {
private _children: Set<DesignTokenNode> = new Set();
private _values: Map<DesignToken<any>, DesignTokenValue<any>> = new Map();
private _derived: Map<DesignToken<any>, DerivedValue<any>> = new Map();
private dependencyGraph: Map<DesignToken<any>, Set<DerivedValue<any>>> = new Map();
private static _notifications: DesignTokenChangeRecordImpl<any>[] = [];

/**
Expand Down Expand Up @@ -261,6 +222,22 @@ export class DesignTokenNode {
: node._values.get(token);
}

private static getOrCreateDependencyGraph(
node: DesignTokenNode,
token: DesignToken<any>
): Set<DerivedValue<any>> {
let dependents = node.dependencyGraph.get(token);

if (dependents) {
return dependents;
}

dependents = new Set<DerivedValue<any>>();
node.dependencyGraph.set(token, dependents);

return dependents;
}

/**
* Emit all queued notifications
*/
Expand Down Expand Up @@ -378,6 +355,9 @@ export class DesignTokenNode {
: DesignTokenMutationType.add;
const prev = DesignTokenNode.getLocalTokenValue(this, token);
this._values.set(token, value);
if (DesignTokenNode.isDerivedFor(this, token)) {
this.tearDownDerivedTokenValue(token);
}
const isDerived = DesignTokenNode.isDerivedTokenValue(value);
const derivedContext = DesignTokenNode.collectDerivedContext(this);
let result: StaticDesignTokenValue<T>;
Expand All @@ -389,12 +369,7 @@ export class DesignTokenNode {
result = value;
}

if (!isDerived && DesignTokenNode.isDerivedFor(this, token)) {
this.tearDownDerivedTokenValue(token);
}

if (prev !== result) {
Observable.getNotifier(this).notify(token);
DesignTokenNode.queueNotification(
new DesignTokenChangeRecordImpl(this, changeType, token, value)
);
Expand Down Expand Up @@ -586,32 +561,25 @@ export class DesignTokenNode {

/**
* Generate change-records for local dependencies of a change record
* TODO: I think we can get some perf gains from making this recursive to reconcile *all*
* tokens from one record input (or maybe a set of inputs?). Instead, dispatch just calls dispatch
* with the result of this, which effectively does the same thing with more steps
*/
private collectLocalChangeRecords<T>(
record: DesignTokenChangeRecordImpl<T>
): Map<DesignToken<any>, DesignTokenChangeRecordImpl<any>> {
const collected = new Map<DesignToken<any>, DesignTokenChangeRecordImpl<any>>();
const { token } = record;

for (const entry of this._derived) {
if (entry[0] !== token) {
const [_token, derivedValue] = entry;
if (derivedValue.evaluator.dependencies.has(token)) {
derivedValue.update();

collected.set(
_token,
new DesignTokenChangeRecordImpl(
this,
DesignTokenMutationType.change,
_token,
derivedValue.evaluator.value
)
);
}
for (const dependent of DesignTokenNode.getOrCreateDependencyGraph(
this,
record.token
)) {
if (dependent.value !== dependent.update().value) {
collected.set(
dependent.token,
new DesignTokenChangeRecordImpl(
this,
DesignTokenMutationType.change,
dependent.token,
dependent.evaluator.value
)
);
}
}

Expand All @@ -630,51 +598,19 @@ export class DesignTokenNode {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it makes sense, but would there be an efficiencies gained by instead sending the list of records to child instead of using nested for loops to send the individual records? Maybe they just have to loop internally anyway and so there's no advantage. Just thought I'd check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Follow on question, is this the dispatch an array improvement slated for a later date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I touched on it in our sync, but I think we can see some improvement by reconciling all changes for each node and dispatching a set of mutations instead of dispatching one mutation at a time. I'll file a tracking issue.

}

private derivedSubscriber: Subscriber = {
handleChange: (subject: DerivedValueEvaluator<unknown>) => {
for (const entry of this._derived.entries()) {
const [token, derivedValue] = entry;

const prev = derivedValue.value;
if (derivedValue.evaluator === subject) {
if (derivedValue.update().value !== prev) {
DesignTokenNode.queueNotification(
new DesignTokenChangeRecordImpl(
this,
DesignTokenMutationType.change,
token,
derivedValue.evaluator.value
)
);
this.dispatch(
new DesignTokenChangeRecordImpl(
this,
DesignTokenMutationType.change,
token,
derivedValue.evaluator.value
)
);

DesignTokenNode.notify();
}
}
}
},
};

private tearDownDerivedTokenValue(token: DesignToken<any>) {
if (DesignTokenNode.isDerivedFor(this, token)) {
const value = this._derived.get(token)!;
if (DesignTokenNode.isAssigned(this, token)) {
Observable.getNotifier(value).unsubscribe(this.derivedSubscriber);
}

value.dispose();
Observable.getNotifier(value).unsubscribe(
this.localTokenChangedSubscriber,
"value"
);

this._derived.delete(token);

value.evaluator.dependencies.forEach(dependency => {
DesignTokenNode.getOrCreateDependencyGraph(this, dependency).delete(
value
);
});
}
}

Expand All @@ -686,24 +622,35 @@ export class DesignTokenNode {
const deriver = new DerivedValue(
token,
DerivedValueEvaluator.getOrCreate(value),
this
);
Observable.getNotifier(deriver).subscribe(
this.localTokenChangedSubscriber,
"value"
this,
subscribeNode
? {
handleChange: () => {
if (deriver.value !== deriver.update().value) {
const record = new DesignTokenChangeRecordImpl(
this,
DesignTokenMutationType.change,
deriver.token,
deriver.evaluator.value
);
DesignTokenNode.queueNotification(record);

this.dispatch(record);
DesignTokenNode.notify();
}
},
}
: undefined
);

if (subscribeNode) {
Observable.getNotifier(deriver.evaluator).subscribe(this.derivedSubscriber);
}
this._derived.set(token, deriver);

deriver.evaluator.dependencies.forEach(dependency => {
if (dependency !== token) {
DesignTokenNode.getOrCreateDependencyGraph(this, dependency).add(deriver);
}
});

return deriver;
}

private localTokenChangedSubscriber: Subscriber = {
handleChange(source: DerivedValue<unknown>) {
// console.log("Need to re-evaluate", source.token)
},
};
}