Skip to content

fix(47872): GoToDefinition won't work if CommonJS module contains import statement on the top of a js file #47904

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

fix(47872): GoToDefinition won't work if CommonJS module contains import statement on the top of a js file #47904

wants to merge 2 commits into from

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #47872

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Feb 15, 2022
@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 15, 2022 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at a7c6d94. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/120164/artifacts?artifactName=tgz&fileId=6F1CEF1CE51B0DB4FAA080653D048E00768299275B3E2C32F406BABBDFB479A202&fileName=/typescript-4.7.0-insiders.20220215.tgz"
    }
}

and then running npm install.

@a-tarasyuk a-tarasyuk marked this pull request as ready for review February 19, 2022 00:07
sandersn
sandersn requested changes Feb 24, 2022
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I want to hold this until we decide whether to the original bug is worth fixing. As-is the PR makes ESM and commonjs syntax completely interchangeable in JS files, which is a big step. It might be the right one, but I want to make sure.

@sandersn sandersn added the Experiment A fork with an experimental idea which might not make it into master label Feb 28, 2022
@a-tarasyuk
Copy link
Contributor Author

The issue #47872 was closed, I think this PR is needless anymore. @sandersn Can I close it?

@sandersn
Copy link
Member

Yes, let's. It's more important than ever to avoid confusing ES and CommonJS modules, and someday CommonJS code won't be that common.

@sandersn sandersn closed this Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandersn sandersn sandersn requested changes

@weswigham weswigham Awaiting requested review from weswigham

Assignees

@weswigham weswigham

Labels
Experiment A fork with an experimental idea which might not make it into master For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(Bug Report): GoToDefinition won't work if CommonJS module contains import statement on the top of a js file
5 participants
@a-tarasyuk @DanielRosenwasser @typescript-bot @sandersn @weswigham