Skip to content

Missing --isolatedModules error for type referenced by decorator metadata #42281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
janwidmer opened this issue Jan 11, 2021 · 3 comments · Fixed by #42915
Closed

Missing --isolatedModules error for type referenced by decorator metadata #42281

janwidmer opened this issue Jan 11, 2021 · 3 comments · Fixed by #42915
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@janwidmer
Copy link

janwidmer commented Jan 11, 2021

Bug Report

Typescript is not throughing an error for Type imports not having the keyword import type from Files containing only type exports, when having isolatedModules set to true.

🔎 Search Terms

  • isolatedModules
  • import type

🕗 Version & Regression Information

  • This is the behaviour in every recent version I tried.

⏯ Playground Link

See attached Zip Code and Readme.md for instructions.

💻 Code

The types.ts files contains only named exports

// types.ts

export interface IGEvent {
	data: {
		eventData: IGEventOptions;
	};
}

The index.ts imports the event interface and uses it within it's decorated method signature

// index.ts
import {Component, EventListener, GondelBaseComponent} from '@gondel/core';
import { IGEvent } from './types';

@Component('HelloWorld')
class HelloWorld extends GondelBaseComponent {

	@EventListener('gEvent')
	_handleEvent(event: IGEvent) {
		alert(event.data.eventData.helloWorld);
	}
}

The used library gondel is a component framework to start components on a page and handle their lifecycle.

The warning goes away when

  • we remove the decorator
  • we define event as any and cast it inside the method to the type IGEvent
  • we import it with import type { IGEvent } from ...

Code Example.zip

🙁 Actual behavior

When using Webpack, it throughs a build-breaking error since Webpack 5, before, a warning was shown.
When just using native TS for Compiling, I don't get an Error.

🙂 Expected behavior

Courtesy of explanation goes to @andrewbranch, see Ticket #40420.

The point of isolatedModules is to tell you ahead of time if a transpiler (like Babel or ts-loader in transpile mode) will not have enough information to know whether the code it produces at runtime is safe or not. That’s what’s happening here—ts-loader in transpile mode cannot possibly know whether IGEvent has a value meaning by only looking at index.ts, and so it stays in. Webpack is smart enough to detect this and tell you about it, and prior to v5, forgiving enough to fix up your runtime code. But at the core, what happened was you wrote something that could not be analyzed by ts-loader, and consequently ts-loader produced fundamentally broken output. isolatedModules is the way you tell the compiler that you’re going to use a transpiler, and so you want errors that prevent you from getting in this situation.

@andrewbranch andrewbranch changed the title TS is not throughing an error for imports without import type with isolatedModules true Missing --isolatedModules error for type referenced by decorator metadata Jan 11, 2021
@andrewbranch
Copy link
Member

This bug is specifically about --emitDecoratorMetadata. The earlier bit of my explanation from #40420:

You have emitDecoratorMetadata set, so TypeScript will want to use IGEvent as part of its metadata annotations if it’s a value. But because the webpack config you’re using is running the compiler as a single-file transpiler, it has no idea that IGEvent doesn’t exist. That code is inherently unsafe in single-file transpilation, which is exactly what import type is for. That said, we ought to be warning you about this in --isolatedModules

@andrewbranch andrewbranch added the Bug A bug in TypeScript label Jan 11, 2021
@andrewbranch andrewbranch self-assigned this Jan 11, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.3.0 milestone Jan 11, 2021
@janwidmer
Copy link
Author

@andrewbranch thanks for updating the ticket and making it more clear :-)

@rbuckton
Copy link
Contributor

I ambiguous imports with --emitDecoratorMetadata explicitly use a "fallback" approach:

__decorate([
    EventListener('gEvent'),
    __metadata("design:type", Function),
    __metadata("design:paramtypes", [typeof (_a = typeof IGEvent !== "undefined" && IGEvent) === "function" ? _a : Object]),
    __metadata("design:returntype", void 0)
], HelloWorld.prototype, "_handleEvent", null);

The problem is that we're not rewriting the import { IGEvent } from "./types" import statement so as not to rely on a possibly missing static binding for IGEvent at runtime.

We possibly should be rewriting this to be the following instead:

var _a;
import { Component, EventListener, GondelBaseComponent } from '@gondel/core';
import * as types_1 from './types';
let HelloWorld = class HelloWorld extends GondelBaseComponent {
    _handleEvent(event) {
        alert(event.data.eventData.helloWorld);
    }
};
__decorate([
    EventListener('gEvent'),
    __metadata("design:type", Function),
    __metadata("design:paramtypes", [typeof (_a = typeof types_1.IGEvent !== "undefined" && types_1.IGEvent) === "function" ? _a : Object]),
    __metadata("design:returntype", void 0)
], HelloWorld.prototype, "_handleEvent", null);
HelloWorld = __decorate([
    Component('HelloWorld')
], HelloWorld);

This way, if there is a value for IGEvent, we can still capture it.

@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
5 participants