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

reimplement deno.enablePatterns feature #153

Closed
wants to merge 8 commits into from

Conversation

turadg
Copy link

@turadg turadg commented Aug 21, 2020

Attempting to add #92 after it was removed from master.

However, in my testing it's not working as expected. Even documents that don't match are getting Deno errors when enabled is true. For example,
Screen Shot 2020-08-21 at 2 48 06 PM

@Flaque did #92 have this problem?


This change is Reviewable

@Flaque
Copy link
Contributor

Flaque commented Aug 21, 2020

@turadg haven't seen it before

@lucacasonato
Copy link
Member

@turadg Some more config checks are happening in https://github.com/denoland/vscode_deno/blob/master/typescript-deno-plugin/src/plugin.ts

@turadg
Copy link
Author

turadg commented Aug 21, 2020

Thanks @lucacasonato . From a quick glance it looks like it'll be tricky to re-use this logic in the plugins module. Plus this method is doing a lot of unnecessary calculation repeatedly.

Taking a step back, should "being enabled" be a concern of the extension itself? I think the problem is that Deno is a special flavor of Typescript (e.g. assuming Deno is defined) but this extension is using the plain 'typescript' language ID. React adds similar assumptions to Javascript and the way it's handled in VS Code is with a new typescriptreact language ID. WDYT of making vscode_deno work the same way?

Then defining the files for which it's enabled is handled by VS Code settings. E.g. to enable everywhere,

"files.associations": {
    "*.ts": "typecriptdeno"
}

or to co-exist with other TS files,

"files.associations": {
    "*.deno.ts": "typecriptdeno"
}

@lucacasonato
Copy link
Member

I don't think we should add our own language ID. We don't change the syntax at all (typescriptreact does because of JSX). We only change semantics.

@turadg
Copy link
Author

turadg commented Aug 28, 2020

We don't change the syntax at all

Deno has its own standard formatting which is different from other TypeScript formatters like Prettier.

We only change semantics.

What's a better way to determine whether a file is a Deno .ts file or a non-Deno .ts file?

I don't think we should add our own language ID.

What are the downsides of adding a language ID? I believe it could inherit from typescript such that it wouldn't diverge except for the specific parts necessary.

@lucacasonato
Copy link
Member

@axetroy what do you think?

@axetroy
Copy link
Contributor

axetroy commented Aug 31, 2020

@axetroy what do you think?

I didn't try this before.

Add a new file type association typescriptdeno. I don't know if it will affect the original typescript function.

@lucacasonato lucacasonato deleted the branch denoland:master November 29, 2020 23:12
@lucacasonato
Copy link
Member

Thanks for the PR, but we are doing a full extension rewrite so this will not be landed anymore.

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.

4 participants