Skip to content

[DEPENDENCY] Pin estraverse to v5.1.0#489

Merged
RandomByte merged 1 commit intomasterfrom
estraverse-5.2.0
Aug 9, 2020
Merged

[DEPENDENCY] Pin estraverse to v5.1.0#489
RandomByte merged 1 commit intomasterfrom
estraverse-5.2.0

Conversation

@RandomByte
Copy link
Copy Markdown
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 estraverse@5.2.0 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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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 estraverse@5.2.0 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 estraverse@5.2.0 [DEPENDENCY] Pin estraverse to v5.1.0 Aug 6, 2020
@RandomByte
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 estraverse@5.1.0.

The package-lock.json is a resolution of the package.json

@codeworrior
Copy link
Copy Markdown
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