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

[DEPENDENCY] Pin estraverse to v5.1.0 #489

Merged
merged 1 commit into from
Aug 9, 2020
Merged

Conversation

RandomByte
Copy link
Member

@RandomByte RandomByte commented Aug 6, 2020

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.

@RandomByte
Copy link
Member Author

Estraverse added support for ChainExpression node (estools/estraverse#113) which is not yet handled by ui5-builder leading to failing tests:

taskRepository: Failed to require task module for generateStandaloneAppBundle: unknown estree node type 'ChainExpression', new syntax?

@RandomByte RandomByte requested a review from codeworrior August 6, 2020 15:25
@coveralls
Copy link

coveralls commented Aug 6, 2020

Coverage Status

Coverage remained the same at 91.513% when pulling 69df35c on estraverse-5.2.0 into e7d4431 on master.

Copy link
Member

@codeworrior codeworrior left a 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
@RandomByte RandomByte changed the title [DEPENDENCY] Update to [email protected] [DEPENDENCY] Pin estraverse to v5.1.0 Aug 6, 2020
@RandomByte
Copy link
Member Author

If you want a quick solution

Yes

I would rather stick to an older version of estraverse

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

Copy link
Member

@codeworrior codeworrior left a 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.

@RandomByte
Copy link
Member Author

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

@codeworrior
Copy link
Member

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.

@RandomByte RandomByte merged commit e5bc455 into master Aug 9, 2020
@RandomByte RandomByte deleted the estraverse-5.2.0 branch August 9, 2020 14:51
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

Successfully merging this pull request may close these issues.

3 participants