Skip to content

Prevents auto import in module: "none" #55556

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

Merged
merged 28 commits into from
Aug 31, 2023

Conversation

hardikkoul
Copy link
Contributor

@hardikkoul hardikkoul commented Aug 29, 2023

Fixes #55256

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Aug 29, 2023
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 29, 2023
@hardikkoul hardikkoul changed the title Prevents auto import in module: "none" #55256 Prevents auto import in module: "none" Aug 29, 2023
@andrewbranch
Copy link
Member

Looks like the test is failing 😕

@hardikkoul
Copy link
Contributor Author

Looks like the test is failing 😕

yeah , just saw that , checking it out.

@hardikkoul
Copy link
Contributor Author

Hey @andrewbranch , as i was not much familiar with the fourslash testing framework, so had to do some research and with some hit and tries the tests are passing now, Please review and verify once if some things need to be changed.

verify.completions({
marker: "",
excludes: ["Thing2A"],
includes: ["!Thing1A"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 What is this?

Copy link
Contributor Author

@hardikkoul hardikkoul Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 What is this?

haha just trying the same thing to exclude it by using include with "!" , read it somewhere if exclude is giving error try including with "!".

@hardikkoul
Copy link
Contributor Author

@andrewbranch , please review and assist me if more changes required as im not good at writing tests

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out the behavior was already working as intended, but it was untested, so I’ve added two tests. --module none only errors when --target is below es2015, and auto-imports were already prevented in that case, and not in the others. (Like I said, --module none is pretty broken and possibly meaningless.) Thanks for your patience!

@hardikkoul
Copy link
Contributor Author

It turns out the behavior was already working as intended, but it was untested, so I’ve added two tests. --module none only errors when --target is below es2015, and auto-imports were already prevented in that case, and not in the others. (Like I said, --module none is pretty broken and possibly meaningless.) Thanks for your patience!

ohhh , thats a relief to know that it was working already, anyway im analyzing the test you wrote , really helpful :) thanks for your help !!

@andrewbranch
Copy link
Member

I would say, “working” here means auto-imports agrees with whether or not --module none allows imports to be written. Whether --module none itself is “working”... 🤷 There have been lot of ideas about what --module none could mean if we wanted to try to make it more consistent:

  • You're never allowed to write module constructs (what you assumed, I think)
  • You're not allowed to write modules, but you can use dynamic import() calls
  • You're not allowed to write modules, but import type imports would be allowed and wouldn't make the file a module
  • Any module constructs you write are allowed, but never transformed to a particular output module format

I think potentially any of these might be on the table for the future, we just haven’t had a reason to pull the trigger on any of them. So the current state is... pretty weird.

@hardikkoul
Copy link
Contributor Author

I would say, “working” here means auto-imports agrees with whether or not --module none allows imports to be written. Whether --module none itself is “working”... 🤷 There have been lot of ideas about what --module none could mean if we wanted to try to make it more consistent:

* You're never allowed to write module constructs (what you assumed, I think)

* You're not allowed to write modules, but you can use dynamic `import()` calls

* You're not allowed to write modules, but `import type` imports would be allowed and wouldn't make the file a module

* Any module constructs you write are allowed, but never transformed to a particular output module format

I think potentially any of these might be on the table for the future, we just haven’t had a reason to pull the trigger on any of them. So the current state is... pretty weird.

omg this is a lot, hahaha and yes i did indeed assume that its possible to write module constructs , current state as it seems is actually weird ngl , hahaha

@andrewbranch andrewbranch merged commit dce7b6d into microsoft:main Aug 31, 2023
@hardikkoul
Copy link
Contributor Author

@andrewbranch , did i just contribute to TypeScript? :D , kind of i guess XD

@HolgerJeromin
Copy link
Contributor

It turns out the behavior was already working as intended,

I do not understand. With typescript ts5.2.2 I still get bogus, file breaking autosuggestions in my modules:none code:
image

Was that changed after branching ts5.2?
At least THIS PR does not fix my #55256

@andrewbranch
Copy link
Member

If you accept the import, do you get an error?

@HolgerJeromin
Copy link
Contributor

Moved the discussion back to the main issue (which I am not allowed to reopen).

snovader pushed a commit to EG-A-S/TypeScript that referenced this pull request Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unwanted auto-imports in script files
4 participants