lodash / babel-plugin-lodash

Modular Lodash builds without the hassle.
Other
1.96k stars 91 forks source link

Poor typing handling #215

Closed alecmev closed 6 years ago

alecmev commented 6 years ago

The following TypeScript code ...

import { template, TemplateExecutor } from "lodash";

const x: TemplateExecutor = template('')

... results in The 'lodash' method 'TemplateExecutor' is not a known module, in both 3.3.2 and the current master, 3.3.3-pre.

Research

Both @babel/plugin-transform-flow-strip-types and @babel/plugin-transform-typescript remove the typing imports in visitor.ImportDeclaration, as seen here, while babel-plugin-lodash goes through the imports in visitor.Program, as seen here.

Problem is, Babel doesn't execute plugins completely sequentially (quick primer, with more details). Instead, it goes through the AST node-by-node, and invokes visitor.* for each node type it encounters, in each plugin, in the order they're defined. visitor.Program gets called before visitor.ImportDeclaration, since it's higher up the AST, so this plugin gets the imports as-is, without any type stripping, regardless of the plugin ordering in .babelrc.

It looks like this plugin is aware of the above, but doesn't address it properly. The master has a heuristic similar to what @babel/plugin-transform-typescript has, but it's missing the crucial isImportTypeOnly check (which would take care of TemplateExecutor), and Flow isn't handled at all. In the meanwhile, the currently-released version is similar to @babel/plugin-transform-flow-strip-types, but it doesn't handle import type/typeof { something } from 'foo' and import { typeof something } from 'foo' correctly, and it isn't able to handle TypeScript at all.

Solution

A potential fix is probably to move as much logic to visitor.ImportDeclaration.exit as possible, so it gets executed after all plugins have run their visitor.ImportDeclaration (AKA visitor.ImportDeclaration.enter). However, I'm not sure if exit doesn't get called if its node got previously removed in an enter.

An alternative fix would be to fully replicate the type-stripping logic, but that's WET and prone to become broken over time.

Just in case, I have never dealt with Babel internals before, so don't take my words at face value.

jdalton commented 6 years ago

Hi @jeremejevs!

If you're up for a PR that would be great. You could also just use lodash-es with webpack 4 to get cherry-picking without babel-plugin-lodash.

alecmev commented 6 years ago

Hi @jdalton!

Thanks for the suggestion, but I can't do that because I want to use lodash/fp, and I need the TemplateExecutor type, which currently isn't exported by @types/lodash-es (though fixable, by doing a ReturnType<typeof template>). I also enjoy using lodash-webpack-plugin.

And why close this issue? Isn't this a bug? Similar to #180, which is actually a bug too.

jdalton commented 6 years ago

Seems more like an enhancement. I'll designate this a dup of #180.