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

Better offline compiler error when accessing private properties #11422

Open
RoxKilly opened this Issue Sep 7, 2016 · 25 comments

Comments

@RoxKilly

RoxKilly commented Sep 7, 2016

I'm submitting a ...

[x] bug report => search github for a similar issue or PR before submitting

Current behavior

I think this is not intended behavior but I apologize if I'm wrong:

I'm trying to move from Just-in-Time to Ahead-of-Time compilation.

1. I've confirmed that the App runs without errors using Just-in-Time compilation. My main.ts is:

enableProdMode();
platformBrowserDynamic().bootstrapModule(AppModule);

2. I clear all my app files from my production folder, but keep 3rd party libraries (eg: Angular 2, Angular 2 Material)

3. I run "node_modules/.bin/ngc" -p ./ This runs with no output to the console. I see an .ngfactory.ts file for each of my .ts components and modules. I also see a .css.shim.ts file for each of my .css that held component styles. In addition, .js and .js.map files have been transpiled and placed in the production directory

4. If I try to run the app at this point, I see 404 not found errors for all the .html files that held component templates

5. I manually move all template files (.html) to production dir and run the App. It runs fine, but it still uses Just-in-Time compilation (255 requests, including compiler.umd.js)

6. I change my main.ts to:

enableProdMode();
platformBrowser().bootstrapModuleFactory(AppModuleNgFactory);

On its own, this makes no difference since the new code has not been compiled. However, if I run ngc again I get lots of errors of the type:

Error at C:/path/to/notify.component.ngfactory.ts:113:41: Property 'visible' is private and only accessible within class 'NotifyComponent'
... (many more like that with lots of properties from lots of components)
Compilation failed

Is this expected when I use private properties? Must all properties be declared public for AoT compilation to work?

Expected behavior

If ngc ran without a hitch before I updated main.ts to use platformBrowser(), I expected ngc to complete successfully after the change.

Reproduction of the problem

Occurs during compilation. Try to AoT compile components with private properties after updating main.ts to use platformBrowser():

constructor(private store:StorageService){}

Please tell us about your environment:

  • Angular version: 2.0.0-rc.6
  • Browser: [all]
  • Language: [TypeScript 2.0.2]
  • Node (for AoT issues):
    node --version = 4.5.0
    npm --version = 2.15.9
@vicb

This comment has been minimized.

Contributor

vicb commented Sep 7, 2016

yep @Input should be public otherwise there is no way to set them.

Leaving this open in order to improve the error message

@vicb vicb modified the milestone: 2.0.1 Sep 7, 2016

@vicb

This comment has been minimized.

Contributor

vicb commented Sep 7, 2016

/cc @chuckjaz

@RoxKilly

This comment has been minimized.

RoxKilly commented Sep 7, 2016

The errors are not just about @Input. Setting an injected service to private will raise the error:

constructor(private store:StorageService){}

Regular class properties as well:

private user:User;

Is this expected as well @vicb ?

@mhevery

This comment has been minimized.

Member

mhevery commented Sep 7, 2016

Yes, AoT generates code. If something is private one can not see it.

@RoxKilly

This comment has been minimized.

RoxKilly commented Sep 7, 2016

@mhevery Thanks Misko

@RoxKilly

This comment has been minimized.

RoxKilly commented Sep 7, 2016

I wondered why only certain properties must be public while other private properties are acceptable. I received an excellent explanation on Stack:

For a given component all its members (methods, properties) accessed by its template must be public in the ahead-of-time compilation scenario. This is due to the fact that a template is turned into a TS class. A generated class and a component are 2 separate classes now and you can't access private members cross-class.

In short: you can't access private members in your templates if you want to use ahead-of-time compilation.

-- pkozlowski.opensource

@vicb vicb changed the title from AoT compiler cannot compile private class properties to Better offline compiler error when accessing private properties Sep 7, 2016

@gjo-rocks

This comment has been minimized.

gjo-rocks commented Oct 19, 2016

But what would be the impact of changing private properties to public...

@aluanhaddad

This comment has been minimized.

aluanhaddad commented Oct 23, 2016

@gjo-rocks
Technically all properties are already public at runtime as the TypeScript compiler removes TypeScript annotations when it emits JavaScript.
The impact is a change in API surface which affects other TypeScript code. I have no idea why NGC behaves this way, it breaks the semantics of TypeScript, implying a subset or fork, and it prevents the language service from even warning you that this is a problem.

Consider you are writing a library using TypeScript and publishing it for consumption, by both TypeScript and JavaScript consumers in a standard way by distributing a .js and a .d.ts file.

You cannot prevent either from accessing private members, but you can provide a better design time experience for your consumer by marking certain properties as private. This allows a TypeScript user to see only the public API surface exposed if they choose to use your .d.ts file, which is completely optional regardless of the consumer's language choice.

None of this affects code emitted by Microsoft's TypeScript compiler. It emits the same code regardless of accessibility modifiers.

That NGC behaves differently from TypeScript implies that when using AOT you are no longer writing in TypeScript.

See #12465

zalog added a commit to zalog/ro-diaspora that referenced this issue Dec 7, 2016

fix(pages/news/news): publică membru folosit în template
Repară eroare de compilare AoT.

"For a given component all its members (methods, properties) accessed by its template must be public in the ahead-of-time compilation scenario. This is due to the fact that a template is turned into a TS class. A generated class and a component are 2 separate classes now and you can't access private members cross-class." -- http://stackoverflow.com/a/39379451/2977191

Detalii:
- angular/angular#11422
- http://blog.mgechev.com/2016/08/14/ahead-of-time-compilation-angular-offline-precompilation/#the-context-property
@tbosch

This comment has been minimized.

Member

tbosch commented Apr 12, 2017

That some properties can be private for bindings is a bug in the way we generate code.
Eventually, we do want to allow private properties, but this requires some changes in our architecture, which we are planning to do this quarter.

@atalis

This comment has been minimized.

atalis commented Jul 18, 2017

I want to clarify the latest comment by @tbosch. Do you mean to say that the bug is in NOT being able to bind private properties, and the work is to allow that? Or the bug is in the fact that some, but not all, of private properties can be bound in the template?

@trotyl

This comment has been minimized.

Contributor

trotyl commented Jul 18, 2017

@atalis A bug is something violating the design.

In current implementation, it's by design to not allow private properties, so throwing error when using private property is the correct behavior and not throwing is a bug, that's tracked at #14739.

But as part of the goal in v5 (or beyond), they want to re-design code generation mechanism. After that, allowing private properties will be the expected behavior.

@chuckjaz

This comment has been minimized.

Member

chuckjaz commented Jul 19, 2017

@atalis It is a bug that some access to private properties are not reported as errors. Because the template is generated in a separate class it doesn't have access to private members of the original class.

In v5 we will be changing how errors in templates are reported that will improve these error messages.

We currently generate code and then use TypeScript to validate that the code we generate type checks. However, we generate code for performance, not type checking so we lose some types and and we are forced to introduce any in places that defeats the type checker (that is, if we didn't inject an any TypeScript would report a spurious error).

Our current plan is to change to generate factories in the transformer phase of TypeScript instead of running the factories through the type-checker and then generate a separate block of code that preserves types (but would perform badly) and map the type error reported in that block back to the part of the template that generated the error.

@esakal

This comment has been minimized.

esakal commented Jul 26, 2017

Reading both @trotyl and @chuckjaz responses I'm not sure whether v5 will allow binding to private properties in the templates or not.

I hope it will allow binding to private properties because I consider the template as part of the component and not as an external consumer. Changing a bindable property used by the template as part of a refactoring should not be treated as breaking change but since v4 forces it to be public I have to assume that someone might be using it and handle it as breaking change of the API.

@chuckjaz

This comment has been minimized.

Member

chuckjaz commented Jul 27, 2017

@eskal v5 will not allow reference to private variables in templates and will report them as an error.

The code for a template is generated in a factory class which is a separate class from the class declaring the template. Only the class declaring the template has access to private members.

For this to change, the template must be considered a member of the class declaring the template. We will not be able to change that for v5 for a variety of reasons but hopefully, we will be able to change this in v6.

@wizarrc

This comment has been minimized.

wizarrc commented Jul 31, 2017

@chuckjaz That's too bad. I was hoping v5 would treat templates as internal to the component class. Since this is the ultimate goal, and it's a bug that allows private members in v4, how hard would it be to make an exception in the type checkers in AOT, etc. to ignore private members inside the template with the goal of removing the exception once it's designed correctly? It's a bit hacky, but going back and forth on this seems like a worse alternative. My guess is that most Angular 2+ devs are using TS and allowing private members is their expectation. Removing it by fixing the bug and then making them wait till v6 to allow them to continue where they left off seems harsh.

@esakal esakal referenced this issue Aug 8, 2017

Merged

KMCNG-37 #181

2 of 2 tasks complete
@trotyl

This comment has been minimized.

Contributor

trotyl commented Oct 14, 2017

@vicb I think supporting this may not be a good idea regarding of this problem, it would cause misleading concept about suggesting to be in the same class, like:

@Component({
  selector: 'my-app',
  template: `
    <div>
      <h2>Hello {{this.name}}</h2>
    </div>
  `,
})
export class App {
  private name: string;
  constructor() {
    this.name = `Angular! v${VERSION.full}`
  }
}

Since private property for this is always accessible in all kinds of languages, supporting this without private property would be a weird behavior.

@talamaska

This comment has been minimized.

talamaska commented Dec 15, 2017

@chuckjaz "v5 will not allow reference to private variables in templates and will report them as an error."
I don't see any errors without aot compilation when using private properties in the template on 5.1. I have a form-field and input based of Material 2.

@trotyl

This comment has been minimized.

Contributor

trotyl commented Dec 15, 2017

@talamaska In JIT mode, the template compilation happens after the TypeScript compilation, so there's no type-checking for template. (Not possible to do that as typings are erased)

In v6ivy, using private property in template would be allowed due to inlined template function (no more ngfactory), and for now you can use $any(this).prop to work-around it.

@talamaska

This comment has been minimized.

talamaska commented Dec 18, 2017

@trotyl where would you call this $any(this).prop? In the template?

@trotyl

This comment has been minimized.

Contributor

trotyl commented Dec 18, 2017

@talamaska Yep, only available after 5.2.0-beta.0.

@chuckjaz

This comment has been minimized.

Member

chuckjaz commented Dec 18, 2017

@wizarrc Sorry for the late reply, for some reason I missed it.

This is a complicated issue, unfortunately. Templates are generated in factory classes that are not the same class as the original component and, therefore, do not have access to the private members of the original class. I agree, however, this is a bit troublesome as the conceptual model is that the template is part of the component itself and should have access to private. TypeScript has no way to express such a "helper" class.

One possible solution is to modify the source that type-checker sees prior to type-checking. This is possible but very slow as it means we would need, at least, two type-checks, one for emitting and one for template validation. Two type checks effectively doubles the time that ngc would take to compiler Angular sources which I don't find acceptable.

All this to say, it is not trivial.

@wizarrc

This comment has been minimized.

wizarrc commented Dec 18, 2017

@chuckjaz Thanks for the reply nevertheless. I didn't realize the whole AST would have to be parsed twice to allow that. I figured there might be a TS hook that could allow custom validation. I agree, it's not worth slowing down the compilation that much for one use case. I found it annoying and tedious but otherwise not too hard to work within the constraints given this should be all fixed anyways in v6.

@BrandonWilliamsCS

This comment has been minimized.

BrandonWilliamsCS commented Nov 27, 2018

In v6, using private property in template would be allowed due to inlined template function...

@trotyl Does this mean "v6 should ship with the ability to use private members in a template", or "we can't address this until after v6"? I ask because I'm using 6.1 and still getting this issue.

Much like esakal, I like to think of the template as being internal to the component and would like to be able to keep properties private when they're only there to share information with the template.

@wizarrc

This comment has been minimized.

wizarrc commented Nov 27, 2018

@BrandonWilliamsCS I think that got pushed back until the Ivy Rendering engine (v3) is complete. I think the v3 engine was originally scheduled to ship with Angular 6, but I'm guessing it won't be turned on by default until Angular 8 or even 9.

@trotyl

This comment has been minimized.

Contributor

trotyl commented Nov 28, 2018

Does this mean "v6 should ship with the ability to use private members in a template"

@BrandonWilliamsCS Sorry I mean in Ivy, the term isn'twasn't much clear at that time. But this isn't a designed feature, just a fact/result of what compilation model in Ivy is.

See other threads for more information: #21706 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment