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

ERROR in RangeError: Maximum call stack size exceeded #29

Open
aaBoustani opened this issue Jan 9, 2018 · 8 comments
Open

ERROR in RangeError: Maximum call stack size exceeded #29

aaBoustani opened this issue Jan 9, 2018 · 8 comments
Assignees

Comments

@aaBoustani
Copy link

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request

Current behavior

I import the module in app.modules and include it in imports.
When I run ng serve --aot, I get an error ERROR in RangeError: Maximum call stack size exceeded.

Environment


Angular version: 5.1.3
Angular Google place version : 0.0.3
 
For Tooling issues:
- Node version: 9.2.1  
- Platform:  Linux Ubuntu-Gnome 17.04

@psykolm22 psykolm22 self-assigned this Jan 25, 2018
@alexzuza
Copy link

alexzuza commented Jan 26, 2018

@psykolm22 Hey!

The reason for this is that your library is released with wrong metadata for AngularGooglePlaceDirective symbol:

"AngularGooglePlaceDirective": {
  "__symbolic": "reference",
  "name": "AngularGooglePlaceDirective"
},

The problem is connected with this issue angular/angular#19219. (Seems it is not completely resolved)

In your library you're exporting AngularGooglePlaceDirective like:

export { AngularGooglePlaceDirective} from './directives/angular-google-place.directive';

https://github.com/psykolm22/angular-google-place/blob/master/src/lib/angular-google-place/index.ts#L14

and also importing the same directive from another path:

import { AngularGooglePlaceDirective } from './directives/index';

https://github.com/psykolm22/angular-google-place/blob/master/src/lib/angular-google-place/angular-google-place.module.ts#L2

I know it's hard to understand but Angular MetadataCollector just cuts metadata if it you have an identifier that refers to different paths. For me it looks like a bug

To workaround it just open angular-google-place/src/lib/angular-google-place/index.ts file and change export as follows:

export { AngularGooglePlaceDirective} from './directives/index';

After that you can open angular-google-place.metadata.json file and now metadata for your directive should look like(prettified version of course:)):

"AngularGooglePlaceDirective": {
      "__symbolic": "class",
      "decorators": [
        {
          "__symbolic": "call",
          "expression": {
            "__symbolic": "reference",
            "module": "@angular/core",
            "name": "Directive"
          },
          "arguments": [
            {
              "selector": "[angularGooglePlace]"
            }
          ]
        }
 ],

/cc @ocombe Can you please look at this?

Update: added minimal repro https://github.com/alexzuza/ng-metadata-bug

@lujakob
Copy link

lujakob commented Jan 29, 2018

@psykolm22 I created a PR for this issue with @alexzuza solution. Please have a look. Would be great if you could update the NPM repo, as it's blocking our production build. Thanks.

@lujakob
Copy link

lujakob commented Jan 29, 2018

In the meantime I have a built package in a public repository that can be used with the following package import
"angular-google-place": "lujakob/angular-google-place-package",

@ray-kay
Copy link

ray-kay commented Jan 30, 2018

thank you so much @lujakob

@ouatrahim
Copy link

@lujakob Thanks for your public repo of angular-google-place! But i don't really get how to add it in my project! It would be awesome if you could give me more informations.

@lujakob
Copy link

lujakob commented Dec 11, 2018

@ouatrahim You add the package by adding the following line to your package.json "dependencies" part:
"angular-google-place": "lujakob/angular-google-place-package"
The usage is described in the readme of the original package https://github.com/psykolm22/angular-google-place

@ouatrahim
Copy link

@lujakob Thanks ! very helpfull!!!

@IRCraziestTaxi
Copy link

Is this really still not fixed? I updated all my packages yesterday in my frantic storm of googling and checking ALL of my files for potential recursive calls, imports, etc. until I FINALLY found this comment. I wasted so much time on this.

rajsite added a commit to ni/nimble that referenced this issue Dec 6, 2021
# Pull Request

## 🤨 Rationale

This PR addresses [TASK 1741886](https://ni.visualstudio.com/DevCentral/_workitems/edit/1741886) and [TASK 1746789](https://ni.visualstudio.com/DevCentral/_workitems/edit/1746789).

The major changes include:
- Removing [barrel](https://basarat.gitbook.io/typescript/main-1/barrel) files
  - After removing barrel files from nimble-angular the library is able to be used in other ViewEngine libraries without resulting in a `Maximum call stack size exceeded` from the angular compiler. 
  - Some possible reasons removing the barrel files helped is eliminating [import cycles](angular/angular#14649) or preventing the same [symbol from being exposed from multiple paths](psykolm22/angular-google-place#29 (comment)). A couple attempts at bisecting did not reveal the exact culprit so barrel files were removed wholesale in this PR.
- Change to stateless directives
  - After switching to ViewEngine builds the `@HostBinding` directives binding to properties that were specific to the nimble components would fail (ie expended state of tree). It seems like that would be [expected generally](angular/angular#13776). Maybe a change in ivy allows for that kind of binding but it is not ViewEngine compatible. 
  - Regardless, this change makes directives stateless instead. This is preferred generally because our custom elements themselves should be managing their state and be the source of truth, not the Angular directive which is just acting as a proxy to the underlying web component.

## 👩‍💻 Implementation

- Barrel files were removed
  - A similar change (but unrelated to the top-level re-factor) was removing the idea of shared control value accessors and instead moving the existing control value accessors to be next to their component. There isn't a strong reason either way but I think it helps keep the components isolated and helps with repo organization even though it may result in some code duplication (very minimal with the current control value accessor patterns).
- Stateless directives
  - Removed existing `@HostBinding` calls and replaced with accessors setting values through the Renderer2 interface as a step to avoid breaking [Angular Universal](https://www.willtaylor.blog/angular-universal-gotchas/) support and 

## 🧪 Testing

- Barrel files were removed
  - Verified against a local build of systemlink-lib-angular with the [changes for nimble-tree-view](https://ni.visualstudio.com/DevCentral/_git/Skyline/pullrequest/228468) applied. Verified that the header was able to perform a production build but not beyond that (ie verifying systemlink-lib-angular using nimble-angular could be used in SystemLinkShared).
- Stateless directives
  - Since we are now managing the definition and state on the directives ourselves (proxying to the web component) it seems necessary to test those code paths instead of relying on the behavior of `@HostBinding`. I modified the nimble-angular boolean directive test to cover the cases I think we should cover and updated the testing docs.
  - One behavior I found writing the tests is that when binding to templates directly (ie as strings in html `<my-elem something="7">`) angular will [assign to the directive property](angular/angular#6919) a string value even if the TypeScript type is something else, like number. See the comments in `template-value-helpers.ts`. Making sure to test this behavior is captured in the test docs updates.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or determined no changes are needed.
rajsite added a commit to ni/javascript-styleguide that referenced this issue Jan 7, 2022
# Justification

Found code that was mixing usages of index imports and directory imports.

For example with the following npm package file structure:
```
- src
  - awesome
    - index.ts
  - other.ts
```

Some imports within the library would use `import './awesome';` while others used `import './awesome/index';`.

In es6 modules world those resolve to two different absolute paths, ie `C:/lib/src/awesome` vs `C:/lib/src/awesome/index` so tooling that may not be aware of commonjs file resolution [may treat them as different symbols](psykolm22/angular-google-place#29 (comment)). This is not a concern in modern versions of tooling that I have seen but enforcing the consistency seems useful anyway for readability.

This PR copies the exisiting airbnb rule for [import/no-useless-path-segments](https://github.com/airbnb/javascript/blob/d8cb404da74c302506f91e5928f30cc75109e74d/packages/eslint-config-airbnb-base/rules/imports.js#L241) and adds the [noUselessIndex](https://github.com/import-js/eslint-plugin-import/blob/7c239fed485ea0785a96c1fa2045d96c181bb79c/docs/rules/no-useless-path-segments.md#nouselessindex) configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants