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

Fail compile when module is Node16/NodeNext while moduleResolution is Node #52428

Closed
5 tasks done
unional opened this issue Jan 26, 2023 · 8 comments
Closed
5 tasks done
Labels
Unactionable There isn't something we can do with this issue

Comments

@unional
Copy link
Contributor

unional commented Jan 26, 2023

Suggestion

When module: Node16|NodeNext and moduleResolution: Node,
the compile pass but the result is invalid.
See: https://github.com/cyberuni/typescript-module-resolutions-demo/blob/main/tests/node/test-result.5.0.0-dev.20230103.md

Either this should be a bug to fix, or mark it as an invalid configuration and bail out.

So that there is no false impression that it works.

🔍 Search Terms

module moduleresolution node16 ReferenceError

This issue seems to be similar, but marked as working as intended: #48835

This issue is about improving the situation to improve UX.

✅ Viability Checklist

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

Related to #52086

Repro steps

pnpm install

# compile all code in repository first
# it will fail, as it tries to build all packages including the tests
# please ignore the errors
pnpm build

# compile the demo
# you will see there are some errors under node16 and nodenext,
# but code for `cjs`, `es-cjs`, `esm-cjs` does not have error,
# i.e. they compiled successfully.
pnpm demo compile node
# you can also run it with `--raw` to see the raw output from `tsc`
pnpm demo compile node --raw


# run the demo
# You will see all code failed with `ReferenceError`
pnpm demo runtime node
@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Jan 30, 2023
@RyanCavanaugh
Copy link
Member

This is a giant markdown file, not a reproducible sample.

@unional
Copy link
Contributor Author

unional commented Jan 30, 2023

Added Repro steps in OP.

The markdown is a summary of the tests, generated by running pnpm demo node.

The steps in repo separate the demo into compile and runtime phase so that it is easier to validate.

The code related to this issue are tests/node/ts/(cjs|es-cjs|esm-cjs).*.ts

@RyanCavanaugh
Copy link
Member

I just have to double-down here on needing a "clearly demonstrated defect" in order to proceed here, as discussed in other issues. I'm not going to poke at every cell of this giant matrix to try to make sense of it. Configuration is a wide-open space and the fact that you see a runtime error in some configuration doesn't necessarily mean that configuration doesn't correspond to some environment where it won't error.

@unional
Copy link
Contributor Author

unional commented Feb 2, 2023

I'm not sure what other information I need to provide here. Please let me know what additional information I can provide.

From #48835, it is marked as Working as Intended based on this comment by @weswigham.

This issue is not a bug report, but a feature request to explicitly fail the compilation so that tsc will not silently compile invalid code successfully.

The steps mentioned in OP reproduced the issue, not just generating the markdown.

The JS file produced are in CJS, instead of ESM as indicated by module: Node16|NodeNext.

After running pnpm demo compile node, you can see the resulting files under ./tests/node/node16/* and ./tests/node/nodenext/*.

@RyanCavanaugh RyanCavanaugh removed the Needs More Info The issue still hasn't been fully clarified label Feb 3, 2023
@RyanCavanaugh
Copy link
Member

The linked repo has dozens of tsconfig files. It has a tool that appears to run TS under hundreds (?) of different configurations.

What I want is a repro that contains a) all the files needed to reproduce the bug you're trying to tell me exists and b) not hundreds of other files as well. Maybe a textual description too.

If literally all you want is for the combination of module: Node16 + moduleResolution: node to be illegal, I'm not sure why there's all this other stuff in the issue report, but anyway, consider that explicitly declined per my prior comment.

@RyanCavanaugh RyanCavanaugh added the Unactionable There isn't something we can do with this issue label Feb 3, 2023
@unional
Copy link
Contributor Author

unional commented Feb 4, 2023

Fine. If you want just this case, here is the repro: https://github.com/cyberuni/TS-52428

If literally all you want is for the combination of module: Node16 + moduleResolution: node to be illegal

Yes, that's what this feature request is about.
And it is exactly what you asked here:

"Please log a specific bug about which line of emit or resolution is wrong."

#52086 (comment)

The original linked repo tries to demonstrate all scenarios when using various settings of the module resolution mechanism in TypeScript.
To answer exactly the question you have raise:

you see a runtime error in some configuration doesn't necessarily mean that configuration doesn't correspond to some environment where it won't error.

It's ok that if you don't like it.

@unional
Copy link
Contributor Author

unional commented Feb 11, 2023

Can we remove the Unactionable label? I think we are clear what is the ask here and it is not unactionable.

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

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

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Unactionable There isn't something we can do with this issue
Projects
None yet
Development

No branches or pull requests

2 participants