Build failed after `estraverse` upgraded to 4.3.0

[zhangliangyi]

Expected Behavior

The ui5 build works.

Current Behavior

The ui5 build is failed.

Steps to reproduce the issue

1. Execute ui5 build command and the error thrown

Context

• UI5 Module Version (output of ui5 --version when using the CLI): {...}
• Node.js Version: 8.12.0
• npm Version: 5.6.0
• OS/Platform: SUSE Linux
• Browser (if relevant): N/A
• Other information: N/A

Affected components (if known)

• [X] ui5-builder
• [ ] ui5-server
• [ ] ui5-cli
• [ ] ui5-fs
• [ ] ui5-project
• [ ] ui5-logger

Log Output / Stack Trace

16:33:14 > Task :au-commonshell:au-commonshell-sap.sf.xlite.min-web:ui5Build FAILED
16:33:14 /tmpfs/jenkins/workspace/V4-PR_au-commonshell_PR-106-TZNXHPMO2RT2XDEYRYSBAOKX7MGM3KVWMEPWI2PLFL2S46VNLH2A/commonshell/au-commonshell-sap.sf.xlite.min-web/node_modules/@ui5/builder/lib/lbt/analyzer/JSModuleAnalyzer.js:179
16:33:14            throw new Error(`unknown estree node type '${type}', new syntax?`);
16:33:14            ^
16:33:14 
16:33:14 Error: unknown estree node type 'ImportExpression', new syntax?
16:33:14     at Object.keys.forEach (/tmpfs/jenkins/workspace/V4-PR_au-commonshell_PR-106-TZNXHPMO2RT2XDEYRYSBAOKX7MGM3KVWMEPWI2PLFL2S46VNLH2A/commonshell/au-commonshell-sap.sf.xlite.min-web/node_modules/@ui5/builder/lib/lbt/analyzer/JSModuleAnalyzer.js:179:10)

Hi colleague,

I am from the SAP Sucessfactors, and we are using the ui5 builder in our daily build. Today we found an issue in ui5-builder and all the UI builds are failed.

Per my investigation, there is a new release of estraverse in several hours ago, it imported a new property called 'ImportExpression'. And I find in the JSModuleAnalyzer.js in ui5-builder, it will check all the properties exposed by estraverse.

https://github.com/estools/estraverse/commit/7fc4475d1b744cc3b90f0abe756c4934884ce8de#diff-16e183a8233505f249340c48ca541fd2

Since the ImportExpression is not defined in the JSModuleAnalyzer.js, an error would be thrown and the build is failed.

[codeworrior]

The error is thrown by some self-protecting check of the JSModuleAnalyzer.

When the set of node types in an ASTree is extended (as it was the case now for dynamic imports) and when the analyzer doesn't know the semantics of the new node type, it could take wrong decisions. To avoid this, it compares its set of know types wit the set of types exposed by estraverse and throws in case of a mismatch.

We just have to think how to better integrate this in our delivery. An update of estraverse now immediately makes all new installations of the tooling fail, which is quite bad and was not the intention of the check. On the other side, the incident shows that the test makes sense. We won't become aware of extensions beforehand.

Maybe we can limit the failure to cases where the new node type really occurs in the analyzed AST or we accept some (suboptimal) default behavior for new nodes (e.g. all code branches are assumed to be unconditional, might result in too many eager dependencies). I just winder how to get feedback then.

[codeworrior]

Mhmm, after naively applying a fix, I see from the travis build failures that our package-lock.json should have prevented this issue.

@zhangliangyi Can you elaborate on how you install the ui5 tooling? @matz3, @RandomByte I guess the only cross package-manager solution on dependency level is to use a fixed dependency (which we won't like) or implementing an alternative strategy in the code (as sketched above)

[zhangliangyi]

@codeworrior I have added some workaround to lock the version of estraverse in the package.json in our components to release the build block. So we can wait for the release of ui5-builder and then remove the workaround .

[petermuessig]

@codeworrior

• what about using this for a daily test execution to detect such kind of changes?
• unfortunately, there is no cross package manager mechanism (AFAIK) to fix dependencies rather than defining them directly in the project, but this would not have helped here as the build tooling is provided as dependencies by an intermediate Successfactors project

[RandomByte]

@codeworrior: I guess the only cross package-manager solution on dependency level is to use a fixed dependency

@petermuessig: unfortunately, there is no cross package manager mechanism (AFAIK) to fix dependencies rather than defining them directly in the project, but this would not have helped here as the build tooling is provided as dependencies by an intermediate Successfactors project

No. Projects should fix their dependencies using lockfiles.

By default, npm always generates a package-lock.json file which locks the versions of all modules used in the project (including transitive dependencies).

Yarn automatically generates a yarn.lock file which serves the same purpose.

The usage of lockfiles prevent issues like the one reported here from occurring in existing projects where no changes to the package.json have been made.
That's why npm and Yarn generate them automatically and it is recommended to check them into your source control.

[RandomByte]

UI5 Builder release v1.4.1 is now compatible with estraverse@4.3.0

[codefactor]

@RandomByte ,

Thanks for the insight into the purpose of package-lock.json

For background, SuccessFactors has hundreds of repositories/applications each having several package.json files, and most of these have references to internal SuccessFactors dependencies that look similar to the following:

{
  "dependencies": {
    "@sf/<name>": "git+https://git@github.xxx.com/<org>/<name>#semver:^1.0.0
  }
}

The usage of semver:^1.0.0 allows us to push changes to hundreds of separate applications/repositories at once without each of the owners of those repositories to do anything, but it wouldn't work if the package-lock.json files were being used. So recently I think we started adding the package-lock.json to .gitignore so that we would always be up-to-date with respect to the declared versions in the respective dependencies. Also, I think there is a bit of code that executes in the Jenkins build that we use that removes the package-lock.json file.

@zhangliangyi ,

Can you help to clarify if I missed something or give more background on this?

I think we might want to reconsider our approach to solving this problem of ours, and maybe we should use package-lock.json files, but instead maybe think of some other gradle code that could re-write the package.json file directly for things like:

1. Updates to internal successfactors dependencies to the latest release tag
2. Synchronize UI5 version numbers

What do you think?