-
Notifications
You must be signed in to change notification settings - Fork 22
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
[DEPENDENCY] Pin estraverse to v5.1.0 #489
Conversation
Estraverse added support for ChainExpression node (estools/estraverse#113) which is not yet handled by ui5-builder leading to failing tests: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want a quick solution, I would rather stick to an older version of estraverse that doesn't support ChainExpression
. For complete support of ChainExpression
, much more has to be done, IMO. E.g. all places that currently react on a MemberExpression
have to be checked whether they should react to a ChainExpression
as well.
And, for ChainExpression
the expression
property becomes conditional, but this can't be expressed correctly with the current mechanisms in the JSModuleAnalyzer
as this is no longer a static quality of the AST node property, but it depends on the optional flag of the current chain element and its parents.
Since the "self-protection" mechanism of our JSModuleAnalyzer [1] is highly dependent on the set of possible node types provided by estraverse, we often saw problems in consuming projects after new releases of estraverse. With [email protected] this is yet again the case. This change forces consumers to use a version of estraverse that is supported by JSModuleAnalyzer (currently 5.1.0). Version updates will only happen through pull requests created by dependabot. [1]: https://github.com/SAP/ui5-builder/issues/309#issuecomment-521108883
21c8678
to
69df35c
Compare
Yes
Got it 👍 It's amazing how I still can't wrap my head around this part of the JSModuleAnalyzer. My fix was solely based on the last one and hopes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has it to be expected that the package-lock.json doesn't change? I thought, it is a super set of the information in the package.json, but maybe it isn't.
The lockfile intentionally does not define ranges for the direct dependencies of the project. Only fixed versions. In this case [email protected]. The package-lock.json is a resolution of the package.json |
That was more or less clear to me. I just thought that the package-lock.json contains enough information to restore the package.json. But that seems not to be the case. Anyhow, I'm fine with the change. |
Since the "self-protection" mechanism of our JSModuleAnalyzer (see https://github.com/SAP/ui5-builder/issues/309#issuecomment-521108883) is
highly dependent on the set of possible node types provided by
estraverse, we often saw problems in consuming projects after new
releases of estraverse. With [email protected] this is yet again the
case.
This change forces consumers to use a version of estraverse that is
supported by JSModuleAnalyzer (currently 5.1.0). Version updates will
only happen through pull requests created by dependabot.