Skip to content
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

TypeScript adds exports {} into code #41513

Closed
jon49 opened this issue Nov 12, 2020 · 20 comments
Closed

TypeScript adds exports {} into code #41513

jon49 opened this issue Nov 12, 2020 · 20 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@jon49
Copy link

jon49 commented Nov 12, 2020

When importing with just types no export {} should be added. And I would also opine that if a person wants to emit export {} they should actually write that into their code themselves or be able to add a hook into the compiler which says to do that. This breaks my browser code. We shouldn't have magical code like this in the compiler. And service workers do not understand the keyword export.

Code

import { Tester } from "./tester"
(async function test() : Promise<Tester> { return { hello: "export" } }())

Expected behavior:

(async function test() { return { hello: "export" }; }());

Actual behavior:

(async function test() { return { hello: "export" }; }());
export {};

Related Issues:

#38696
#38713
https://stackoverflow.com/questions/64798609/typescript-appends-export-to-end-of-file-in-typescript-4-but-not-typescript-3

@jon49
Copy link
Author

jon49 commented Nov 12, 2020

I was thinking a better approach would have been to add a flag to the tsconfig.json file like alwaysAddExport: true rather than breaking browser based code. As of now it seems like the best way would be to make a breaking change in TypeScript 5 and move this change back and adding in a flag. In the interim it would be nice to have a flag that says isBrowserCode: true that will stop the auto creation of export {}.

I don't know all the ins and outs of TypeScript and the decisions made but this decision seems highly irregular for code that should also work in the browser and in Service Worker scripts.

@MartinJohns
Copy link
Contributor

the best way would be to make a breaking change in TypeScript 5

TypeScript does not follow SemVer.

@jon49
Copy link
Author

jon49 commented Nov 12, 2020

@MartinJohns Oh, I didn't know that. Like I said. I don't follow everything which is done in the TypeScript world, otherwise I would have to be a full time employee. But it is nice to know. I think at this point most people assume SemVer for projects. So, that is a bit disheartening to hear that TypeScript doesn't follow SemVer.

@MartinJohns
Copy link
Contributor

So, that is a bit disheartening to hear that TypeScript doesn't follow SemVer.

Almost every update had a breaking change. Whether they call it TypeScript 41.0 or TypeScript 4.1 doesn't really make much of a difference. You can read about the heated discussion at #14116.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Nov 12, 2020

And service workers do not understand the keyword export.

According to this page, they should if you include { type: "module" } - does that work?

navigator.serviceWorker.register('/outputFile.js', {
  type: 'module'
});

@jon49
Copy link
Author

jon49 commented Nov 12, 2020

I'll check it out tonight. Regardless, this would be more of a work around. TypeScript should not be adding additional unneeded and unexpected code to our code base. This seems more like a tooling problem and should be an config setting change.

@jon49
Copy link
Author

jon49 commented Nov 13, 2020

This is what Firefox says when adding the export {} to the end of a script file that isn't a module. As an error and it breaks the code.

Uncaught SyntaxError: export declarations may only appear at top level of a module

And in Chrome:

index.js:2 Uncaught SyntaxError: Unexpected token 'export'

So, long term this really needs to be fixed since it is clearly a bug on TypeScript's side.

@RyanCavanaugh
Copy link
Member

Aside: Even if we did follow semver, this wouldn't warrant a major version bump, because it's a change that increases our compliance with the ECMAScript spec.

Anyway the file you wrote was a module (since it had an import), and we were emitting a file that was not a module (since it lacked export / import), so that's a bug. It's always been the intended behavior that we don't change the moduleness of a file during emit, this was just a missed case that never should have worked this way in the first place.

You're free to suggest a new compiler option but this is the intentional behavior.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Nov 13, 2020
@jon49
Copy link
Author

jon49 commented Nov 13, 2020

It only had a type import. How else are we supposed to import types? Do we need to make all our types globals?

@jon49
Copy link
Author

jon49 commented Nov 13, 2020

Type imports are not the same as importing actual JavaScript files. Regardless, I don't think TypeScript should be creating random code in our code base. Even if it is a module if I don't want to export anything I should have some random export {} appended to my file.

If the way forward is to make everything global, that is fine. The behavior of injecting random code into our code bases doesn't seem right though.

@RyanCavanaugh
Copy link
Member

It's not "random"; it's preserving the moduleness of the file. You wrote code that didn't contribute to the global scope because the containing file was a module, then TypeScript emitted a file that did contribute to the global scope -- that's an obvious, obvious defect.

@jon49
Copy link
Author

jon49 commented Nov 14, 2020

I wrote code that imported types only. It is not a module though. It's unfortunate that TypeScript no longer supports importing types for files that are not modules. If that is what you think is best that's fine. I think it is unfortunate that is all. I don't think TypeScript should be adding that type of code into our code bases. If someone want to export {} they should do it themselves. But like I said. This is not my project and you guys own it. So, if you think it is best then that is fine. But it seems like this is tooling problem and TypeScript should not be adding additional code that people don't want in their code base. But if you think otherwise that is your choice and it is fine. I'll just have to find a work around so TypeScript no longer breaks my code. Maybe I'm the edge case, there are others that are having issues with this too. But it just seems like if you want to export {} then you should just type it out rather than making the compiler break everyone else's code.

It would be nice if there was a clean way to import types without having to resort to the global scope. Granted I'll have to test that out to see if that will work. It seems like TypeScript is breaking its mission here by catering to some developers over others though.

@DanielRosenwasser
Copy link
Member

It would be nice if there was a clean way to import types

It's not great, but there's this:

type Type = import("./path").Type

@jon49
Copy link
Author

jon49 commented Nov 14, 2020

Thanks I'll try it out.

I was thinking that part of what bothers me about this is that adding export {} to all module files seems like a dirty hack to get around a build problem rather than figuring out a more elegant way to do it. I know we live in a messy world and not all solutions can be elegant so I'm not faulting the person who came up with it. I just think maybe some more thought should have gone into it rather than breaking a lot of people's code and forcing people that like the cleaner build to have to change their code. And adding unneeded code to people's code base which will have to be stripped out later on by code minifiers (other code which will need to be created/modified because of this change).

And I tested this out:

   <script type=module>
      console.log("cats!")
   </script>

This is a perfectly valid module code. No need to have export {} to identify it. It seems like this is purely for tooling on Webpacks side. And who knows how long before Webpack goes to the graveyard? Or when it will start being used less and less? I would imagine TypeScript would still be around for the long haul but Webpack could start seeing less and less use over the long run and we'll still have this pretty useless artifact in the output of TypeScript.

My vote is to have to have a setting to opt in to have export {} added to the code since it is unexpected artifact to the output of a person's code base, is unneeded, would need to be stripped back out by another tool if a person is looking have an absolute minimum go over the wire, and is purely needed only for code bases which use Webpack as a build tool.

But you guys to whatever you like. I just think this seems dirty, it's not a clean solution to the problem.

I think as a temporary solution to this I'll just stay on 3.9.* until, hopefully, this is fixed in a later version of TypeScript.

@jon49
Copy link
Author

jon49 commented Nov 14, 2020

I did some more testing and these seem to be valid module scripts (they work just fine in Firefox):

index.js:

console.log("I like dogs")

index.html:

<script type=module>
   import { } from "./index.js"
   console.log("Cats!")
</script>

Result in console:

I like dogs!
Cats!

So, export {} is not needed.

@rosdi
Copy link

rosdi commented Nov 14, 2020

@jon49 , according to the explanation by RyanCavanaugh above, the old behaviour in TypeScript 3.9 is actually a bug, the current new behaviour in TypeScript 4.0 (where export {} is emitted), is the correct behaviour.

So don't count for this to be reverted. The best we can hope for is a new compiler switch.

I guess we have to change our workflow now, we can no longer simply link a transpiled js file directly in a web page.

@jon49
Copy link
Author

jon49 commented Nov 15, 2020

According to ECMAScript, if I understand it correctly. It says you don't even need anything in a module. So, the export {} statement is not required by JavaScript and is purely for tooling. It's NOT added for creating correct JavaScript modules. See https://tc39.es/ecma262/#sec-module-semantics-runtime-semantics-evaluation

I tried type Type = import("./path").Type but that creates a global type. So, you can't use it in multiple places.

Changing the settings in the tsconfig.json file to module: commonjs - as suggested in the Stack Overflow question - adds additional unwanted code also.

Service Workers don't support modules yet, at least not in Chrome, see: https://bugs.chromium.org/p/chromium/issues/detail?id=824647

So, basically, if you want to share types in your code, and you want to keep those types local to where they you created them and you are not using modules you need to go old school, like in C with header files. So, you would need to do something like this:

my-file.d.ts
my-file.ts

And then export those types to a global type file like this:

Global.d.ts

import { MyType as MyType_ } from "../some-dir/my-file.d.ts"

global {
    declare namespace SomeDir {
         type MyType = MyType_
    }
}

Which is pretty onerous. So, I hope you can see why I'm pushing against this so hard.

Now, this is my current solution which keeps me on the latest TypeScript version. I just delete the incorrect code which TypeScript injects into my code. This uses PowerShell but can be written an any language.

param([string] $Path)

$stream = [IO.File]::Open($Path, [IO.FileMode]::Open)
$stream.Position = $stream.Length - 12
$bytes = 0..12 | % { $stream.ReadByte() }
if ("$bytes" -eq "101 120 112 111 114 116 32 123 125 59 13 10 -1") {
    $stream.SetLength($stream.Length - 12)
    Write-Host "Removed from $Path"
}
$stream.Close()
$stream.Dispose()

Personally, I consider this to be a bug on TypeScript's side, since it doesn't make sense to force a JS file to be a module just because it is importing or exporting types and since it is not required for the file to be a module to have any exports. But even if you don't consider it to be a bug; It is a rough change for those that use TypeScript purely for the types and nothing else.

My vote is that a setting be added which would be explicitExpots: true/false. By having to opt into explicit exports also makes TypeScript friendlier for those just getting started with TypeScript. It also makes it so nothing unexpected happens when you are transpiling your code.

@KeithHenry
Copy link

Service workers and web workers are close to supporting modules, but don't yet. Not all browsers are 100% compliant with the module spec.

In my case TS 4.0 is adding export {}; to web workers that import type definitions (none of those make it to the JS output). They are not loaded as modules because not all our users have support for it. Previous versions of TS did not break this file, the new version of TS does.

Why is TS adding an export to a file with only import type and that isn't imported by anything else? I thought import type was supposed to have no effect on output?

I find the "export {}; is spec compliant so tough" viewpoint expressed above really confusing. I though outputting practical JS that works in real browsers from nice clean spec-compliant TS code was one of the fundamental reasons to use a transpiler like TS in the first place?

I don't really care whether this is opt-in or default behaviour with an opt-out, either would be fine, but the current solution appears to be either:

  • Downgrade TS (and keep team + devops chain on old version), or
  • Add something external to fix the output from TS so that it doesn't break workers, or
  • Split the project up into multiple sub-projects and write alternate tsconfig.json files for workers that allow import type but don't export {}; (I'm not sure it's even possible and this is particularly painful for my team because we are using Visual Studio and its support for multiple tsconfig is really poor), or
  • Remove the import type declarations (in which case why use TS at all?)

None of these is a good option.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Nov 19, 2020
@RyanCavanaugh RyanCavanaugh reopened this Nov 19, 2020
@RyanCavanaugh
Copy link
Member

Actually let's just track this at #41567 since there's a good discussion already about the scenario and implications of such a flag

Offroaders123 added a commit to Offroaders123/Smart-Text-Editor that referenced this issue Oct 15, 2022
Rewrote the Service Worker in TypeScript! It *kind of* works great, but not quite. I wish the TypeScript team made tooling for Workers a bit better, as it seems harder to work with than it should be.

This rewrite was all from last night, and it took a few hours to learn how to get things right (I didn't get around to commiting it until now though). There aren't any type errors anymore, but I'm also unsure if it will run sucessfully yet. I don't have a device/OS to test the Web Share Target API support anymore, since my Chromebook runs Ubuntu now :)

While it's not fully functional yet, this is a step in the right direction for type-safe Service Worker code!

I mostly rewrote it with my new programming styles, expanding things out for readability, and making use of async-await where it was previously just `.then()` calls and such.

Some help to get it working:
https://github.com/NicholasPeretti/service-worker-ts-example
https://stackoverflow.com/questions/89332/how-do-i-recover-a-dropped-stash-in-git (I had an oopsie XD)

Edit:
Tried testing out the worker in the browser, and modern TypeScript now includes empty `export {}` instances in the resulting JS, which breaks declaring the file as a module, as you can't have `export {}` in a non-module script. The only way I got close to this was to name the file `.mts`, but I'm not sure if I like that or not. But hey, it may work. With that being a possible solution, I found all of these sources along the way also:
https://stackoverflow.com/questions/56356655/structuring-a-typescript-project-with-workers
https://github.com/jakearchibald/typescript-worker-example (now outdated, unfortunately, thanks to this new TypeScript change)
microsoft/TypeScript#41513
https://www.devextent.com/create-service-worker-typescript/ (almost works! falls down to the same issue sadly)
microsoft/TypeScript#14877
microsoft/TypeScript#11781
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants