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

refactor(compiler): cleanup and preparation for integration #4314

Closed
wants to merge 2 commits into from

Conversation

tbosch
Copy link
Contributor

@tbosch tbosch commented Sep 22, 2015

  • Rename DirectiveMetadata into CompileDirectiveMetadata, merge
    with NormalizedDirectiveMetadata and remove ChangeDetectionMetadata
  • Store change detector factories not as array but
    directly at the CompiledTemplate or the embedded template
    to make instantiation easier later on
  • Already analyze variable values and map them
    to Directive.exportAs
  • Keep the directive sort order as specified in the
    @View() annotation
  • Allow to clear the runtime cache in StyleCompiler
    and TemplateCompiler
  • Ignore script elements to match the semantics of the
    current compiler
  • Make all components dynamically loadable and remove
    the previously introduced property @Component#dynamicLoadable
    for now until we find a better option to configure this
  • Don’t allow to specify bindings in @View#directives and @View#pipes as this was never supported by the transformer (see below for the breaking change)

BREAKING CHANGE:

  • don't support DI bindings in @View#directives and @View@pipes any more in preparation of integrating the new compiler. Use @Directive#bindings to reexport directives under a different token instead.

Part of #3605

]);
});

it('should camel case variables', () => {

Choose a reason for hiding this comment

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

@tbosch now that we don't run the compiler on the DOM parsed by a browser, what are you thoughts on relaxing case-insensitivity rules imposed by a browser. In this particular example, do we want to let people write var-someA and be it equivalent to var-some-a?

Same applies to property names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you create an issue explaining the details?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkozlowski-opensource Well, the compiler can still be run in the browser, in which case we will use the regular DOM compiler...

Copy link
Contributor

Choose a reason for hiding this comment

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

@pkozlowski-opensource IMO deviating from the standard would really be confusing here. Also parse5 lowercase attribute names.

@tbosch does that mean that we will have 2 different compilers - the regular and the offline ones ?

Choose a reason for hiding this comment

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

@tbosch @vicb right, I'm also not keen on deviating from the standard. But:

  • people are having problems with snake-case to camelCase convention
  • I was under the impression that we are going to use the same compiler in a browser an during the build. In other words I was assuming that we parse strings and not the live DOM, even in the browser.

So, I second @vicb question here: will we have 2 different compilers - the regular and the offline ones ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new compiler has 2 code paths: for runtime and for codegen. These code paths share most of their implementation and also tests! The main difference is that the runtime compiler does not codegen but creates the artifacts directly, i.e. by calling new on them.

@@ -58,10 +64,10 @@ var BIND_NAME_REGEXP =

const NG_CONTENT_ELEMENT = 'ng-content';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove these!

@tbosch tbosch added pr_state: LGTM action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Sep 22, 2015
@tbosch
Copy link
Contributor Author

tbosch commented Sep 22, 2015

Sorry, wrong person. Reviewed in person with @yjbanov

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

tbosch and others added 2 commits September 22, 2015 11:52
- Rename `DirectiveMetadata` into `CompileDirectiveMetadata`, merge
  with `NormalizedDirectiveMetadata` and remove `ChangeDetectionMetadata`
- Store change detector factories not as array but
  directly at the `CompiledTemplate` or the embedded template
  to make instantiation easier later on
- Already analyze variable values and map them
  to `Directive.exportAs`
- Keep the directive sort order as specified in the
  `@View()` annotation
- Allow to clear the runtime cache in `StyleCompiler`
  and `TemplateCompiler`
- Ignore `script` elements to match the semantics of the
  current compiler
- Make all components dynamically loadable and remove
  the previously introduced property `@Component#dynamicLoadable`
  for now until we find a better option to configure this
- Don’t allow to specify bindings in `@View#directives` and `@View#pipes` as this was never supported by the transformer (see below for the breaking change)

BREAKING CHANGE:
- don't support DI bindings in `@View#directives` and `@View@pipes` any more in preparation of integrating the new compiler. Use `@Directive#bindings` to reexport directives under a different token instead.

Part of angular#3605
@tbosch tbosch closed this in cc0c304 Sep 22, 2015
@tbosch tbosch deleted the compiler-cleanup branch September 22, 2015 19:53
robwormald pushed a commit to robwormald/angular that referenced this pull request Sep 25, 2015
- Rename `DirectiveMetadata` into `CompileDirectiveMetadata`, merge
  with `NormalizedDirectiveMetadata` and remove `ChangeDetectionMetadata`
- Store change detector factories not as array but
  directly at the `CompiledTemplate` or the embedded template
  to make instantiation easier later on
- Already analyze variable values and map them
  to `Directive.exportAs`
- Keep the directive sort order as specified in the
  `@View()` annotation
- Allow to clear the runtime cache in `StyleCompiler`
  and `TemplateCompiler`
- Ignore `script` elements to match the semantics of the
  current compiler
- Make all components dynamically loadable and remove
  the previously introduced property `@Component#dynamicLoadable`
  for now until we find a better option to configure this
- Don’t allow to specify bindings in `@View#directives` and `@View#pipes` as this was never supported by the transformer (see below for the breaking change)

BREAKING CHANGE:
- don't support DI bindings in `@View#directives` and `@View@pipes` any more in preparation of integrating the new compiler. Use `@Directive#bindings` to reexport directives under a different token instead.

Part of angular#3605
Closes angular#4314
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: no
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants