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

Improve typescript_tsdk lookup #748

Merged
merged 1 commit into from
Jun 8, 2020
Merged

Improve typescript_tsdk lookup #748

merged 1 commit into from
Jun 8, 2020

Conversation

alecmev
Copy link
Contributor

@alecmev alecmev commented May 26, 2020

Fix #538

This is flawed, of course, since the server used is never updated once initialized, so gotta restart ST when switching to/from a project with a custom typescript_tsdk set, but I can't afford to spend time on a refactor to accommodate this, and this is still much better than the status quo.

@msftclas
Copy link

msftclas commented May 27, 2020

CLA assistant check
All CLA requirements met.

Comment on lines +100 to +101
log.warn("Path of tsserver.js: " + proc_file)
log.warn("Path of tsc.js: " + get_tsc_path())
Copy link
Contributor Author

@alecmev alecmev Jun 1, 2020

Choose a reason for hiding this comment

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

log.warn because now tsdk_location deduction is a bit more involved, and it's likely that more people will want to see what's going on. I've considered doing log.debug when typescript_tsdk setting isn't set, but the savings in noise are likely not worth the extra confusion of conditional logging.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the log, I agree a bit less with skipping the conditional check, but it's not enough to block the PR 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was that people might think that they have a tsdk_location in the settings of their current project, while in fact they don't, and the missing log lines could slightly complicate the troubleshooting.

@alecmev
Copy link
Contributor Author

alecmev commented Jun 1, 2020

@orta @DanielRosenwasser Please, take a look, this fixes the most upvoted open issue.

@orta
Copy link
Contributor

orta commented Jun 1, 2020

Conceptually this looks good, I need to get a working ST3 dev env again to verify this all works before I can merge - I might have time for that tomorrow 👍

@alecmev
Copy link
Contributor Author

alecmev commented Jun 8, 2020

@orta Friendly ping 😉

Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

I gave this a run with my dev copy of the compiler and it worked well, as well as testing without a typescript_tsdk and it resolved correctly to the built-in versions:

2020-06-08 16:41:17,996: 4575129024: WARNING: Path of tsserver.js: /Users/ortatherox/Library/Application Support/Sublime Text 3/Packages/TypeScript/tsserver/tsserver.js
2020-06-08 16:41:17,996: 4575129024: WARNING: Path of tsc.js: /Users/ortatherox/Library/Application Support/Sublime Text 3/Packages/TypeScript/tsserver/tsc.js
error: Package Control

Comment on lines +100 to +101
log.warn("Path of tsserver.js: " + proc_file)
log.warn("Path of tsc.js: " + get_tsc_path())
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the log, I agree a bit less with skipping the conditional check, but it's not enough to block the PR 👍

@orta orta merged commit a0d393a into microsoft:master Jun 8, 2020
@orta
Copy link
Contributor

orta commented Jun 8, 2020

Thanks for the ping!

@alecmev
Copy link
Contributor Author

alecmev commented Jun 9, 2020

Thank you for dedicating time to this!

@orta
Copy link
Contributor

orta commented Feb 23, 2021

We reverted these changes, after looking at it we realized that the constant lookback for the tsdk could be a cross-user code eval security hole ( C:/ is writable for all users on a machine )

@sahilrajput03
Copy link

We reverted these changes, after looking at it we realized that the constant lookback for the tsdk could be a cross-user code eval security hole ( C:/ is writable for all users on a machine )

What ..?

image

So how come we'll use the react jsx now, provide help please!!

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.

Typescript-Sublime-Plugin should allow per-project specification of typescript_tsdk
5 participants