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

(feat) support ts 4.7 group aware organize import #1515

Merged
merged 10 commits into from
Jun 19, 2022

Conversation

jasonlyu123
Copy link
Member

@jasonlyu123 jasonlyu123 commented Jun 10, 2022

#1486

Added a check in the svelte2tsx to add a blank line if the import matches the import organization group in the ts 4.7. Also fixed a bug introduced when updated to ts4.7 where an empty indent would be added sometimes. Not sure if there's another way to fix it.

We could probably use the group-aware organize import for #1508 if we add a blank line to separate module script import with instance import. Is this something we want to support?

@dummdidumm
Copy link
Member

Maybe we should support #1508 because this is what happens now:

<!-- before -->
<script context="module">
	import { beforeNavigate } from '$app/navigation';

	import { setResponse } from '@sveltejs/kit/node';

	onDestroy;
</script>

<script>
	import { onDestroy } from 'svelte';
	beforeNavigate;

	setResponse;
</script>

<!-- after -->
<script context="module">
	import { beforeNavigate } from '$app/navigation';

	import { setResponse } from '@sveltejs/kit/node';
	import { onDestroy } from 'svelte';

	onDestroy;
</script>

<script>
		beforeNavigate;

	setResponse;
</script>

Note how the script import is added to the last group of the context script. Is this the behavior a user would want? I'm not sure. Then again I'm not sure if users will miss the current behavior of grouping all these imports in one script. Tough decision..

@jasonlyu123
Copy link
Member Author

jasonlyu123 commented Jun 14, 2022

Side note. Because of the new group-aware organize import I found some behaviour differences between Linux / Mac and Windows. This caused the test to fail when the group didn't need organizing. Maybe it's the cause of #1269. Because we always use \n between the moved import. Maybe that is the reason why Typescript always think it needs organizing. To normalize the line ending.

I removed the semicolon in some previous tests so that it always has the edit. But maybe we should find a way to filter it out.

Edit: well.. this won't help with the test because we have eol=lf in gitattribute.

@dummdidumm
Copy link
Member

dummdidumm commented Jun 14, 2022

Mhhm yeah how to test this? 😅 Are we able to somehow hijack reading the file during tests and insert the line breaks in a way we want? If we can't fix it through this, then maybe we can fix #1269 by applying the changes ourselves and then check if the code is still the same.

@jasonlyu123
Copy link
Member Author

Wondering if we can check the line ending on the original file. If the file is all CRLF or all LF then we use it. If the file has a mixed line ending or no line break we use the system line ending. This should at least help folks using LF in Windows and make our test more reliable.

And then we can try to #1269 later. I tried it on a ts file and it seems like the ts extension also just applies the edit even if nothing changes. So maybe that's not that important.

If we want to go this route do we also apply it to other ts features that accept format options? If so we probably can work around the failed test here. And do it in another PR.

@dummdidumm
Copy link
Member

If we want to go this route do we also apply it to other ts features that accept format options? If so we probably can work around the failed test here. And do it in another PR.

Sounds good 👍 What are other areas where we need this in? If we do need this elsewhere, we should extract the "what kind of line ending to use?"-logic into a small function (in another PR then, doesnt have to be now).

Is this PR good to merge now? Seems like you added the logic and the tests look good.

@jasonlyu123
Copy link
Member Author

Yeah. We probably can extract it out later. I think it's ready to merge.

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.

None yet

2 participants