Webiny-js: Bug: Explicit type export creates an error on importing, and the build is suppressing the error

Created on 17 Sep 2020  路  19Comments  路  Source: webiny/webiny-js

This is:

  • Bug


Specifications

  • version: 4.12.0

Expected Behavior


The build should notify about an 'illegal' operation. (from babel-typescript's view)

Actual Behavior


It compiles, and it will fail to build on import from npm in an external project.

Steps to Reproduce the Problem



Import the UI button from 4.12.0 in an external project and try to build it.

Detailed Description


Known 'issue' with babel-typescript https://github.com/babel/babel/issues/8361

In my recent PR https://github.com/webiny/webiny-js/pull/1239
This 2 lines created the issue: https://github.com/webiny/webiny-js/pull/1239/files#diff-ea8b3f94ab9796095149971cce7f86bcR2-R3
The babel-typescript way is to export * from something, instead of named exporting a type.

Possible Solution


https://devblogs.microsoft.com/typescript/typescript-and-babel-7/
Microsoft suggest that on babel build, the isolatedModules flag should be enabled.
I tried to enable the flag, and I've got several errors from build.

Current example:

This file: https://github.com/webiny/webiny-js/blob/e8061c223ddaeaf69e1aa736ca51b963f55e6eab/packages/ui/src/Button/index.ts
Compiles to this:
index.js

export { ButtonProps, ButtonFloatingProps, ButtonDefault, ButtonPrimary, ButtonSecondary, ButtonFloating, ButtonIcon } from "./Button";
export * from "./IconButton/IconButton";
export * from "./CopyButton/CopyButton";
//# sourceMappingURL=index.js.map

index.d.ts

export { ButtonProps, ButtonFloatingProps, ButtonDefault, ButtonPrimary, ButtonSecondary, ButtonFloating, ButtonIcon } from "./Button";
export * from "./IconButton/IconButton";
export * from "./CopyButton/CopyButton";

The dts is just fine, but the js contains the type export also, which creates a build error on build from external project.

stale-issue

All 19 comments

A quick fix for the current problem could be to remove the explicit type export.

https://github.com/webiny/webiny-js/blob/e8061c223ddaeaf69e1aa736ca51b963f55e6eab/packages/ui/src/Button/index.ts#L1-L9

export * from "./Button";

this seems to fix the current build problem, but there is several files exporting type explicitly, but those didn't caused problems with build (as far as I know)

Interesting 馃

Yeah.. this is also what's happening in packages/ui/src/Icon/index.ts:2, otherwise the error might appear there as well:

image

The other files that you mentioned, not causing errors, are probably not exporting types, but just actual components.

export * is fine with babel-typescript, check the issue from babel I linked

No problem @latotty, I'm just glad the fix wasn't too crazy and hard-to-debug, but that's very much thanks to you, so thanks a lot again for helping and providing valuable resources and knowledge. 馃殌 馃殌

I will also close the #1249 once 4.12.1 is up, which should be as soon as the CI/CD is done 馃檪 .

But of course, will leave https://github.com/webiny/webiny-js/issues/1248, very interested what that discussion might bring to the table.

I'll get back to you and @jcode-hub soon as this is done.

Will close this one @latotty.

Will close this one @latotty.

Sure, but the issue is still on 馃槃 and can happen in the future again, so I think the isolatedModules flag should be investigated.

I understand @latotty.

I'll bring the issue back, and maybe we can discuss it with the other one you posted today (https://github.com/webiny/webiny-js/issues/1248).

But hopefully, the CI should catch these errors in the future. Not sure why it didn't happen with this particular case, maybe it was a bug in the CI config. I'll have to investigate that.

Thanks again for your help!

@latotty the issue you linked (https://github.com/babel/babel/issues/8361) also references a https://github.com/babel/babel/pull/11171 which allegedly adds support for named type exporting. That should've been available since [email protected] 馃. Please check what version of babel is being installed in your project. Sure would love to figure this out.

@Pavel910 I use 7.10, so it is fine on my part, but webiny builds the packages with 7.0 so it is vulnerable to this kind of issues.
If I understand the PR correctly after babel 7.9 we could use export type in typescript so it wouldn't compile the types into the js files.

@latotty what do you mean 7.0? I have @babel/[email protected] locally on my machine. This is what we have in our package.json:

"@babel/cli": "^7.0.0",
"@babel/core": "^7.0.0",

That means anything greater or equal to 7.0.0 but less than 8 - which will always pull in the latest available version.

@Pavel910 Ohh yeah, sorry, got used to exact types in my projects 馃槃
So then the webiny packages can be changed to use the export type syntax.

@latotty so what's the verdict here? So far we never had problems with this, your PR is the first one that hit this. Would you like to submit another PR to fix that Button type export, or....?

@Pavel910
My problem is that we can't export types explicitly currently in a webiny package. Because webiny uses [email protected], which doesn't support export type yet. But event if the ts is upgraded to 3.8, a big problem will be still there.
The build system does not notify the developers about the mistake, that they exported a type as a value, and babel compiles that into the js file, which generates a build error in the library users.

@latotty I'm confused now. Is it babel's or TS's problem? We talked about babel not supporting it, then we figured out that 7.9.0 should support it 馃

@Pavel910
First, babel will never support this kind of statement:

import { MyType } from './type';
export { MyType };

this will always compile to a faulty code.
The solution is to enable isolatedModules fllag in typescript, so the tsc will throw an error if someone exported a type as normal export by mistake.

Babel offers 2 solutions to export types from an other module.
One is the export * from './type'; it will work.
The other one is the new typescript syntax the export type which is supported from babel 7.9 and typescript 3.8.

I don't know if isolatedModules and export type works together, never tried.

@latotty Great, thanks for the summary! We're planning to upgrade to TS 4 for v5 and we'll take this issue into account and see what to do about it.

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

Was this page helpful?
0 / 5 - 0 ratings