Amphtml: Error in `undefined` when running `gulp unit` on `action-impl.js`

[caroqliu]

What's the issue?

Error is repeatedly surfaced when running gulp unit --local_changes after modifying src/service/action-impl.js:

Error in `undefined`: TypeError: Cannot read property 'name' of undefined
Error in `undefined`: TypeError: Cannot read property 'name' of undefined
...

Example Travis build

Appears to be related to findImports in the gulp unit task

How do we reproduce the issue?

1. Trivially modify src/service/action-impl.js
2. Run gulp unit --local_changes

cc @ampproject/wg-infra

[estherkim]

Thanks for filing this! Happy to take a look.

Possibly related - https://github.com/ampproject/amphtml/pull/27341

[samouri]

The error is indeed within the find-imports-forked package. Specifically when it runs babel.transformFileSync. I ran a bisect and isolated the issue to this commit: f801a93c2f079ae439e9a819b09bde0f028f3028

[samouri]

Ah. In https://github.com/ampproject/amphtml/pull/27576 we've become stringent about who can "call" babel. It is reasonable, but has also uncovered that find-imports calls babel. Unfortunately it doesn't also label itself via caller.

The error in Travis comes from this line:
https://github.com/ampproject/amphtml/blob/master/babel.config.js#L59

[estherkim]

Nice find @samouri! I decided to switch out find-imports-forked for dependency-tree which is smaller and better supported. Luckily, it doesn't use babel.

PR coming in a sec!

[samouri]

I was curious how it could work without parsing to an ast. It still uses babel...but skips straight to the parser (https://github.com/dependents/node-source-walk/blob/master/index.js#L1).

Switching to it should solve our issue since it'll bypass the config flow. I'm worried that if it doesn't pay attention to our custom babel configs, it could end up breaking if we adopt nonstandard syntax. Since we probably won't, maybe thats fine?

cc @jridgewell for babel-fu advice

[rsimha]

Thanks for the sleuthing, @estherkim and @samouri! I'm the guilty party who added the stringent checks in #27576 for who can call babel. This was done to make sure we didn't inadvertently use the wrong config in our compilation toolchain. However, the checks don't account for the fact that dev dependencies could use babel, and CI checks didn't block #27576 because it didn't modify any runtime code, and therefore, didn't run unit tests with --local_changes.

Switching to it should solve our issue since it'll bypass the config flow. I'm worried that if it doesn't pay attention to our custom babel configs, it could end up breaking if we adopt nonstandard syntax. Since we probably won't, maybe thats fine?

For this use case (figuring out the set of runtime files imported by each test file), I think it's safe to not use any of our custom babel configs.

It is reasonable, but has also uncovered that find-imports calls babel. Unfortunately it doesn't also label itself via caller.

@jridgewell Instead of throwing an error below, should we just return an empty config when an unrecognized caller is detected? Or do you think that will make our compilation toolchain error prone once again, say, when a new code path is introduced?

https://github.com/ampproject/amphtml/blob/2e097a66e7790d6ed9c0f799613b1a961f40b176/babel.config.js#L58-L67

[samouri]

We should also handle the case where caller is undefined. Even if just to throw a better error

[jridgewell]

Instead of throwing an error below, should we just return an empty config when an unrecognized caller is detected? Or do you think that will make our compilation toolchain error prone once again, say, when a new code path is introduced?

I think we can return an empty config, while also logging an error message?

[rsimha]

Reopening this because find-imports-forked is.... forked! It depends on a ton of @babel packages, several of them implicitly, and frequently breaks when the monorepo is upgraded.

Newest errors (Travis logs):

Error in `undefined`: Error: Cannot find module '@babel/plugin-syntax-import-meta' from '/home/travis/build/ampproject/amphtml'
Error in `undefined`: Error: Cannot find module '@babel/plugin-syntax-import-meta' from '/home/travis/build/ampproject/amphtml'
Error in `undefined`: Error: Cannot find module '@babel/plugin-syntax-import-meta' from '/home/travis/build/ampproject/amphtml'
Error in `undefined`: Error: Cannot find module '@babel/plugin-syntax-import-meta' from '/home/travis/build/ampproject/amphtml'
Error in `undefined`: Error: Cannot find module '@babel/plugin-syntax-import-meta' from '/home/travis/build/ampproject/amphtml'
Error in `undefined`: Error: Cannot find module '@babel/plugin-syntax-import-meta' from '/home/travis/build/ampproject/amphtml'
Error in `undefined`: Error: Cannot find module '@babel/plugin-syntax-import-meta' from '/home/travis/build/ampproject/amphtml'
Error in `undefined`: Error: Cannot find module '@babel/plugin-syntax-import-meta' from '/home/travis/build/ampproject/amphtml'