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

Output consistency across platforms #181

Merged

Conversation

olemarius
Copy link
Contributor

Fixes #180

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Nov 20, 2023

Can you provide images of the before and after the change you did? Thanks

This would make it easy for other maintainers to review

@olemarius
Copy link
Contributor Author

newline inconsistency (fixed in this PR)
image

file order inconsistency (fixed in this PR, but needs to be confirmed by someone on unix based platform):
image

Also, quotes inconsistency (NOT fixed in this PR)
image

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Nov 20, 2023

That is eslint problem we should migrate to v2 of antfu configuration

and ignore registry build files in eslint.config.js

@olemarius
Copy link
Contributor Author

@sadeghbarati ok, so windows users should just build registry and push even with windows newline and other inconsistencies ?

@sadeghbarati
Copy link
Collaborator

I'm not familiar with node too much but I saw the pain of windows path in other projects 🫠

@olemarius
Copy link
Contributor Author

@sadeghbarati yea me neither but thanks to unjs/pathe it was super easy to swap, ref #179 . AFAIK unjs has a bunch of tools like this to make it easier to deal with different platforms.

@zernonia
Copy link
Member

zernonia commented Nov 21, 2023

Yeah the registry script was designed to be run on unix based operating system. I've checkout the branch and ran the registry, seems to be getting the same result as on dev branch. @olemarius I believe this change would allow windows user to run the script, and not affecting unix user right? 😁

@olemarius
Copy link
Contributor Author

@zernonia from what I could see, the index.ts file was always in the bottom of the file list, even when the other files started with a letter after the letter "i".

I added .sort() that should sort it alphabetically both on windows and unix, but if you're getting the same results on dev I'm not sure if the sort is applied ?

There's an issue posted on readdir Windows sorting inconsistency here nodejs/node#3232

@sadeghbarati
Copy link
Collaborator

@olemarius You are Windows user right? If you want to test on unix system just open you GitHub fork in StackBlitz playground

https://stackblitz.com/github/olemarius/shadcn-vue/tree/output-consistency-across-platforms

@olemarius
Copy link
Contributor Author

Did some adjustment and found that .sort() will put lowercase after uppercase, from chatgpt:

The reason 'index.ts' is placed last in the sorted array is because JavaScript's sort() method uses Unicode code point order to sort strings by default. In Unicode, uppercase letters come before lowercase letters, and special characters and numbers come before letters. In this case, 'index.ts' starts with a lowercase letter ('i'), and the other strings start with uppercase letters ('T'). That's why 'index.ts' comes after the other strings when sorted in Unicode order.

index.ts is now last both on Windows and on Unix (tested on StackBlitz), so that Windows users will update only files that actually changed.

Windows:
image

But www/registry/index.ts uses double quotes both on stackblitz and windows. Is it converted to to singleqotes via lint staged?

apps/www/package.json Outdated Show resolved Hide resolved
Co-authored-by: Sadegh Barati <sadeghbaratiwork@gmail.com>
Copy link
Collaborator

@sadeghbarati sadeghbarati left a comment

Choose a reason for hiding this comment

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

Finally tested and checked with Source Control changes, it had the same output as Unix-based environments.

@sadeghbarati sadeghbarati merged commit ef3ec54 into unovue:dev Dec 31, 2023
1 check failed
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.

[Feature]: output consistency across platforms
3 participants