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

Remove the check that if base file name starts with ^ is dynamic file name since those files can exist #36109

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

sheetalkamat
Copy link
Member

Fixes #35734

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.

Code looks OK, but do we lose any ability to detect or create dynamic file names since real files can use carat now?

@sheetalkamat
Copy link
Member Author

do we lose any ability to detect or create dynamic file names since real files can use carat now?

We already have tests for all the scenarios that use dynamic file names and they work, (I think work to retain projectRootPath for open files has rendered this not needed any more)

@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 10, 2020

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 4858f4b. You can monitor the build here. It should now contribute to this PR's status checks.

@sheetalkamat
Copy link
Member Author

@uniqueiniquity @jessetrinity @amcasey can you try our the drop here to see the debugging scenarios are working ok.
I tried creating bunch of dynamic files in VS but I might not have covered all scenarios. (file is created dynamic when debugging in ie and clicking files from solution explorer and script documents list if there is not matching file found on disk.. eg you compile ts with --inlineSources and then delete the ts files)

@uniqueiniquity
Copy link
Contributor

@sheetalkamat sure thing - I actually just built it locally a few moments ago.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 10, 2020

Hey @sheetalkamat, 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/59993/artifacts?artifactName=tgz&fileId=8AAE0DD5296BDA99EDEF45782C5550B9CDEE791E097459DD7D16A0FBB90E9F6802&fileName=/typescript-3.8.0-insiders.20200110.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@uniqueiniquity
Copy link
Contributor

@sheetalkamat Looks good to me. Tried naming normal files with a leading ^, tried using dynamic files, and tried using dynamic files that start with a leading ^. All seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debug Failure. False expression: in openExternalProjects in getOrCreateScriptInfoWorker
4 participants