feat: export collection of built-in directives #1524

pkozlowski-opensource · 2015-04-24T12:26:53Z

This PR makes it slightly easier to list built-in directives in @View and addresses part of #1293 based on this comment

IMO this change is backward-compatible, non-controversial and makes it easier to implement other boilerplate-busters discussed in #1293.

pkozlowski-opensource · 2015-04-24T12:41:29Z

Hmm, it looks like Dart is not happy about export {CSSClass, For, If, NonBindable, Switch, SwitchWhen, SwitchDefault};...

Any ideas on rewriting it so Dart is not complaining? Or is it a bug in the Dart transpiler that should be fixed?

mhevery · 2015-04-24T21:45:53Z

modules/angular2/directives.js

 export * from './src/directives/class';
 export * from './src/directives/for';
 export * from './src/directives/if';
 export * from './src/directives/non_bindable';
 export * from './src/directives/switch';
+
+export var BUILT_IN_DIRECTIVES = [For, If, NonBindable, Switch, SwitchWhen, SwitchDefault];


It should not be all caps. How about coreDirectives?

@View({ directives: [coreDirecitives, MyDirective] })

Also it needs a doc (with an example on how to use it) so that it will show up in the documentation.

pkozlowski-opensource · 2015-04-27T12:59:11Z

@mhevery changed the name to coreDirecitives and added docs.

mhevery · 2015-04-28T23:34:08Z

modules/angular2/directives.js

+ * })
+ * @View({
+ *   templateUrl: 'myComponent.html',
+ *   directives: [coreDirecitives, OtherDirective]


typo coreDirectives

mhevery · 2015-04-29T22:06:49Z

@alexeagle or @mprobst can you help @pkozlowski-opensource with the const issue above. See #1283

alexeagle · 2015-04-29T22:09:55Z

handing to @mprobst

mprobst · 2015-04-29T22:18:48Z

@pkozlowski-opensource so as discussed in offline, we don't have a way to mark an expression as const (in the Dart sense) from TypeScript source code. I could special case const $var = $expr; to translate as const $var = const $exp;, but I think that's wrong in the general case. const ~= final in Dart, and we might have const $var = ...; expressions that should really translate to final $var = ...;.

So two options:

Use two files. Have export var coreDirectives = [For, If, ...]; in foo.ts, and const coreDirectives = const [For, If, ...]; in foo.dart.
Invent some way in ts2dart to recognize expressions that should be const. We cannot put decorators (e.g. @CONST) onto expressions, so it'd have to be some uber ugly hack like const var x = /* ~~~const mode~~~ */ ( expr );. I'd rather not do that.

tbosch · 2015-04-30T01:15:45Z

How about we use static fields on classes that have a @const annotation?

On Wed, Apr 29, 2015 at 3:18 PM Martin Probst notifications@github.com
wrote:

@pkozlowski-opensource https://github.com/pkozlowski-opensource so as
discussed in offline, we don't have a way to mark an expression as const
(in the Dart sense) from TypeScript source code. I could special case const
$var = $expr; to translate as const $var = const $exp;, but I think
that's wrong in the general case. const ~= final in Dart, and we might
have const $var = ...; expressions that should really translate to final
$var = ...;.

So two options:

Use two files. Have export var coreDirectives = [For, If, ...]; in
foo.ts, and const coreDirectives = const [For, If, ...]; in foo.dart.

Invent some way in ts2dart to recognize expressions that should be
const. We cannot put decorators (e.g. @const) onto expressions, so it'd
have to be some uber ugly hack like const var x = /* ~~~const mode~~~ */
( expr );. I'd rather not do that.

—
Reply to this email directly or view it on GitHub
#1524 (comment).

mprobst · 2015-04-30T01:24:20Z

We could do that, but it might still be odd and hacky as the const part is not the field, but rather the expression going into it. The separate files approach, one for dart, one for ts, should make this easy enough to solve, with not too much duplication (four lines of code or so). It's IMHO also easier to understand/maintain.

tbosch · 2015-04-30T21:11:11Z

Yes, but having the outer class is actually a nice place to keep constants.

E.g.
@const
class AngularDefaultDirectives {
static BASICS = [For, If];
static FORMS = [...]
}

On Wed, Apr 29, 2015 at 6:24 PM Martin Probst notifications@github.com
wrote:

We could do that, but it might still be odd and hacky as the const part is
not the field, but rather the expression going into it.

The separate files approach, one for dart, one for ts, should make this
easy enough to solve, with not too much duplication (four lines of code or
so). It's IMHO also easier to understand/maintain.

—
Reply to this email directly or view it on GitHub
#1524 (comment).

mhevery · 2015-05-01T04:45:25Z

I don't think const on static is a good idea. As you want to import foo not Bar.foo. I think @mprobst comment hack is reasonable.

var x = /*cost*/ [1,2];

mprobst · 2015-05-01T17:11:21Z

I really don't like the comment hack, experience shows that's in general a terrible idea.

What about just putting that stuff into a foo.dart file for the const expressions, and foo.ts for the TypeScript variant?

pkozlowski-opensource · 2015-05-01T17:44:54Z

@mprobst yeh, the comment trick doesn't sound super-clean. I will split things into separate Ts / Dart files for this PR. But I think that we will need some kind of solution for this use-case as I keep bumping into it and there is another issue (#1283) where I will need to play it again. I saw also number of other places in the source code where we don't use consts due to this very problem.... In short: it is recurring.

alexeagle · 2015-05-05T23:31:25Z

Have we considered making a feature request for TypeScript to add the matching semantics with some available keyword or other syntax?

mhevery · 2015-05-06T05:17:19Z

@alexeagle I don't think that you could ever implement dart-const semantics in JS. So this is for us to solve.

I am not a fan of comment, but duplicate code sounds even worse.

Any other suggestions?

tbosch · 2015-05-06T15:54:39Z

We could add a magic function
On Tue, May 5, 2015 at 10:17 PM Miško Hevery notifications@github.com
wrote:

@alexeagle https://github.com/alexeagle I don't think that you could
ever implement dart-const semantics in JS. So this is for us to solve.

I am not a fan of comment, but duplicate code sounds even worse.

Any other suggestions?

—
Reply to this email directly or view it on GitHub
#1524 (comment).

tbosch · 2015-05-06T16:07:32Z

Ups, didn't finish my last mail:

We could have a function function CONST(a) { return a; }. In JavaScript,
this is all there is to it.
However, the dart transpiler would recognize this and make all expressions
that are used in the argument const.

E.g.

export const BASICS = CONST([For, If]);

In Dart, this would transpile into:

const BASICS = CONST(const [For, If]);

On Wed, May 6, 2015 at 8:54 AM Tobias Bosch tbosch@google.com wrote:

We could add a magic function
On Tue, May 5, 2015 at 10:17 PM Miško Hevery notifications@github.com
wrote:

@alexeagle https://github.com/alexeagle I don't think that you could
ever implement dart-const semantics in JS. So this is for us to solve.

I am not a fan of comment, but duplicate code sounds even worse.

Any other suggestions?

—
Reply to this email directly or view it on GitHub
#1524 (comment).

pkozlowski-opensource · 2015-05-08T18:08:54Z

We need to land dart-archive/ts2dart#151 and update deps on ts2dart before this PR can land

angular-automatic-lock-bot · 2019-09-06T19:22:59Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

vsavkin added the in progress label Apr 24, 2015

googlebot added the cla: yes label Apr 24, 2015

pkozlowski-opensource force-pushed the def_directives_collection branch 5 times, most recently from e668ce1 to d69324b Compare April 24, 2015 14:10

mhevery reviewed Apr 24, 2015
View reviewed changes

pkozlowski-opensource force-pushed the def_directives_collection branch from d69324b to a395702 Compare April 27, 2015 12:58

mhevery reviewed Apr 28, 2015
View reviewed changes

mhevery added @lgtm action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 28, 2015

pkozlowski-opensource mentioned this pull request Apr 29, 2015

Automatically load/inject the most commonly used directives into all templates #1586

Closed

mhevery modified the milestone: M8: GT Customer (non-blocking) Apr 30, 2015

alexeagle mentioned this pull request May 5, 2015

fix(forms): export collection of form directives as const in Dart #1648

Closed

mprobst mentioned this pull request May 8, 2015

feat(const): Translate CONST_EXPR to const expressions. dart-archive/ts2dart#151

Merged

feat: export collection of built-in directives

a5988b3

pkozlowski-opensource force-pushed the def_directives_collection branch from a395702 to a5988b3 Compare May 11, 2015 14:05

pkozlowski-opensource closed this in a5638a9 May 11, 2015

vsavkin removed the in progress label May 11, 2015

pkozlowski-opensource deleted the def_directives_collection branch July 20, 2015 12:55

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: export collection of built-in directives #1524

feat: export collection of built-in directives #1524

pkozlowski-opensource commented Apr 24, 2015

pkozlowski-opensource commented Apr 24, 2015

mhevery Apr 24, 2015

mhevery Apr 24, 2015

pkozlowski-opensource commented Apr 27, 2015

mhevery Apr 28, 2015

mhevery commented Apr 29, 2015

alexeagle commented Apr 29, 2015

mprobst commented Apr 29, 2015

tbosch commented Apr 30, 2015

mprobst commented Apr 30, 2015 via email

tbosch commented Apr 30, 2015

mhevery commented May 1, 2015

mprobst commented May 1, 2015

pkozlowski-opensource commented May 1, 2015

alexeagle commented May 5, 2015

mhevery commented May 6, 2015

tbosch commented May 6, 2015

tbosch commented May 6, 2015

pkozlowski-opensource commented May 8, 2015

angular-automatic-lock-bot bot commented Sep 6, 2019

feat: export collection of built-in directives #1524

feat: export collection of built-in directives #1524

Conversation

pkozlowski-opensource commented Apr 24, 2015

pkozlowski-opensource commented Apr 24, 2015

mhevery Apr 24, 2015

Choose a reason for hiding this comment

mhevery Apr 24, 2015

Choose a reason for hiding this comment

pkozlowski-opensource commented Apr 27, 2015

mhevery Apr 28, 2015

Choose a reason for hiding this comment

mhevery commented Apr 29, 2015

alexeagle commented Apr 29, 2015

mprobst commented Apr 29, 2015

tbosch commented Apr 30, 2015

mprobst commented Apr 30, 2015 via email

tbosch commented Apr 30, 2015

mhevery commented May 1, 2015

mprobst commented May 1, 2015

pkozlowski-opensource commented May 1, 2015

alexeagle commented May 5, 2015

mhevery commented May 6, 2015

tbosch commented May 6, 2015

tbosch commented May 6, 2015

pkozlowski-opensource commented May 8, 2015

angular-automatic-lock-bot bot commented Sep 6, 2019