Skip to content

Conversation

@mathieumure
Copy link
Contributor

@mathieumure mathieumure commented Mar 5, 2021

Fix #92

I figured out that passing typescript module in settings is too heavy and produces overhead in other preset such as eslint-config-airbnb-base.

In this PR, I load typescript only once to avoid overhead.

@dummdidumm
Copy link
Member

This should be a backwards compatible change to not break existing users, so it would need to check if a boolean was passed in. Also I don't like that now there's a require of TypeScript inside the code base, the API was designed that way to avoid that. I instead suggest something like 'svelte3/typescript': () => require('typescript') which would also achieve the lazy loading behavior you are looking for.

The other thing is I don't quite understand how this will help performance. If you use TypeScript, it will be used either way at some point, so if you pay the cost upfront or later on shouldn't matter right?

@Conduitry
Copy link
Member

I skimmed @dummdidumm's comment, and then was about to say "If we have to lazy load, I'd much prefer something like () => require('typescript') where it remains the config file's responsibility to do the loading" and then I saw that that is exactly what he suggested.

I too am confused about where the overhead would be coming from here. Can you explain more what's going on? Is the config file re-evaluated (without a require cache) a bunch of times for different plugins?

@mathieumure
Copy link
Contributor Author

Hello @Conduitry and @dummdidumm,

Thanks for the quick reply 👍
You are right and I just realized that my code is not backward compatible after creating the PR 😅
I just correct accordingly to your suggestions.

I'm still trying to figure out what are the rules slowing down by this setting.
My guess is that some rule recursively runs through all keys in settings and the `require('typescript') is really big.
So far, I find that it's linked to import related rules but I don't have the reason why.

@dummdidumm
Copy link
Member

dummdidumm commented Mar 8, 2021

In #95 it came up to do the true option like you initially proposed. In that issue @Conduitry seemed to be okay now with going that route after checking back with the eslint maintainers. If he confirms, then you could switch back the implementation to the first one. Sorry for the back and forth 😅

@mathieumure
Copy link
Contributor Author

@dummdidumm ha ha ha ok. I'll do it when @Conduitry will confirm it 😉.

@mathieumure
Copy link
Contributor Author

@dummdidumm I add a boolean option with the functional one. Tell if you are okay with this.

@JounQin
Copy link

JounQin commented Mar 10, 2021

@dummdidumm I add a boolean option with the functional one. Tell if you are okay with this.

Or add string option too?

@mathieumure
Copy link
Contributor Author

@dummdidumm I add a boolean option with the functional one. Tell if you are okay with this.

Or add string option too?

What do you intend to do with this string ? 🤔 Do you have any use case ?

@JounQin
Copy link

JounQin commented Mar 11, 2021

@mathieumure I want to use ttypescript instead.

@mathieumure
Copy link
Contributor Author

@JounQin 🤔 I think we can achieve this in another PR because IMO it's a new feature.

@mathieumure
Copy link
Contributor Author

@Conduitry did you have the time to check this PR ?

@Conduitry Conduitry merged commit 994a537 into sveltejs:master Apr 23, 2021
@Conduitry
Copy link
Member

This has been released (with some adjustments in) 3.2.0. Thank you!

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.

Plugin very slow when using with other preset

4 participants