Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Declarative DSL #63

Merged
merged 6 commits into from
Oct 3, 2016
Merged

Declarative DSL #63

merged 6 commits into from
Oct 3, 2016

Conversation

novemberborn
Copy link
Contributor

@novemberborn novemberborn commented Sep 27, 2016

Implements most of #33. Supports app-action, app-actions, app-element, app-store and app-widget (for widget registration).

Builds on #62.

@novemberborn novemberborn changed the base branch from master to extract-refactorings September 28, 2016 11:23
The following custom elements are recognized:

* `<app-action>`
* `<app-actions>`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to add <app-element>.

@codecov-io
Copy link

codecov-io commented Sep 28, 2016

Current coverage is 100% (diff: 100%)

Merging #63 into master will not change coverage

@@           master   #63   diff @@
===================================
  Files          11    12     +1   
  Lines         700   920   +220   
  Methods         2     3     +1   
  Messages        0     0          
  Branches      128   191    +63   
===================================
+ Hits          700   920   +220   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update e513f95...8d71d94

@novemberborn novemberborn changed the base branch from extract-refactorings to master September 30, 2016 09:24
@novemberborn novemberborn force-pushed the declarative-dsl branch 2 times, most recently from d7d511f to dc4a181 Compare September 30, 2016 09:30
'an ID is required'() {
projector.innerHTML = '<app-widget></app-widget>';
return rejects(app.realize(root), Error, 'Cannot resolve widget for a custom element without \'data-uid\' or \'id\' attributes');
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need this test.

@@ -307,9 +305,6 @@ export default function realizeCustomElements(

let promise: Promise<WidgetLike> = null;
if (isWidgetInstance) {
if (!id) {
throw new Error('Cannot resolve widget for a custom element without \'data-uid\' or \'id\' attributes');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need to throw.

Perhaps change message to app-widget requires data-uid or id attribute.

@novemberborn
Copy link
Contributor Author

Pushed a fixup for my earlier comments, and a big refactor of extractRegistrationElements which came about as I was fixing strict null check violations in another branch (that already contained this merge).

Copy link
Member

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

Just a few comments...

};
}

export interface Result {
Copy link
Member

Choose a reason for hiding this comment

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

At a minimum we should have JSDOC style comments on anything we export...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, pushing fixups to add comments for new exports.

}
else {
resolve(module);
require([toAbsMid(mid)], resolve);
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider some sort of AMD loader detection and fail gracefully if not support... We might want to also structure this in a way that it is possible that we might environmentally detect other loaders in the future and attempt to code path for resolution there.

Copy link
Member

Choose a reason for hiding this comment

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

We have dojo-core/load which is designed for loading resources without having to specifically worrying about what loader, but it doesn't specifically load modules (e.g. run them in a context once retrieved)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like dojo-core/load falls back to the available require method, so that would work for modules right? That would be a follow-up change though.

Copy link
Member

Choose a reason for hiding this comment

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

#68 covers this now

Copy link
Member

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

👍 (once we sort out CI)

@novemberborn
Copy link
Contributor Author

(once we sort out CI)

https://travis-ci.org/dojo/app/builds/164598735 failed due to socket hangups, and uploadCoverage is failing. So I'm pretty sure CI passes…

Support resolving specific, non-default members, as well as
(non-default) module contents.

Document exports and resolution behavior.
Avoid unnecessarily initializing variables in tests to remove strict
null check violations. Silence others.

Refactor parsing logic and task typings. This reduces ambiguity and the
need for type casts, especially if strict null checks are taken into
account.

Use a helper method to read attributes. The various task interfaces are
declared with optional properties. An object containing undefined
property values conforms to such an interface. Use the helper method to
return undefined values, rather than null, so non-existing values can be
used for optional properties.

Ensure task property values are not null. Null values cannot be assigned
to optional interface properties.

Cast known-string pop() result to string. Splitting a string always
results in an array with at least one string element, so popping from
that array will never return undefined.

Throw when action, store and widget elements have from and factory
attributes.
@novemberborn novemberborn merged commit 86faf1d into master Oct 3, 2016
@novemberborn novemberborn deleted the declarative-dsl branch October 3, 2016 12:51
@dylans dylans added this to the 2016.10 milestone Oct 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants