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

chore: Use moduleDetection set to force in create-next-app TS templates #50693

Open
wants to merge 1 commit into
base: canary
Choose a base branch
from

Conversation

icyJoseph
Copy link
Contributor

@icyJoseph icyJoseph commented Jun 2, 2023

For Contributors

What?

Up to Typescript 4.9.5, having a module with no imports/exports was labelled as an error. Because of the isolatedModules
flag being set to true.

However, as of 5.x.x, this has changed, and how TypeScript detects modules is not the same anymore.

I came into contact with this behaviour change in: #50684, I've expanded a bit more in that discussion.

Why?

In TS 5.x.x a file like this:

// foo.ts
interface Foo {
  bar(): void;
}

Makes it so that we can access Foo anywhere. Unless we opt-in this file into module land (with an import or export expression/statement).

This was not possible up to 4.9.5, where it'd fail with error:

'foo.ts' cannot be compiled under '--isolatedModules' because it is considered a global script file. Add an import, export, or an empty 'export {}' statement to make it a module.

1 interface Foo {
  ~~~~~~~~~

How?

This PR proposes introducing "moduleDetection": "force",, to opt-in files as modules, in projects made with create-next-app.

This is not final of course. There's a reason why the TS has introduced this change:

Scripts are no longer errors in isolatedModules unless they use global namespace declarations, which is the only kind of construct that is not safe to single-file transpile. See the motivating issue #46295, design discussion in #51813, and implementation at cf5c284 as part of #52203.

Perhaps we should include "moduleDetection": "auto", instead, which is the default, and append a section to the README, saying that to avoid the issue described above, one has to change that "force".

One has to also wonder if there's actually any potential "danger" of types becoming globally available like that.

I tried to include everything I've been able to gather with respect to this "change in behaviour".

Please feel free to dismiss and close the PR unilaterally if you think it is a non-issue.

@icyJoseph icyJoseph requested a review from a team as a code owner June 2, 2023 13:45
@ijjk ijjk added the create-next-app Related to our CLI tool for quickly starting a new Next.js application. label Jun 2, 2023
@ijjk
Copy link
Member

ijjk commented Jun 2, 2023

Allow CI Workflow Run

  • approve CI run for commit: 5476200

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-next-app Related to our CLI tool for quickly starting a new Next.js application.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants