-
Notifications
You must be signed in to change notification settings - Fork 537
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(check:policy): New policy to check and sort tsconfig files in the repo #20076
Conversation
@@ -52,9 +50,11 @@ | |||
// "experimentalDecorators": true, /* Enables experimental support for ES7 decorators. */ | |||
// "emitDecoratorMetadata": true, /* Enables experimental support for emitting type metadata for decorators. */ | |||
"incremental": true, | |||
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */ | |||
"esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */, | |||
/* Basic Options */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basic Options banner moved - banners are not preserved. Those should be reviewed 1 by 1.
I was also thinking that target is pretty key and may deserve to be sorted early. lib might be in the same bucket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the banner moving is a bug in sort-jsonc. But I don't know if it could realistically be fixed since the comment has to anchor to a "real node" in the JSON. In this case it gets anchored to "target" so it moves with it. (Sorry if this was obvious). That seems reasonable behavior but not ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that it moves. I'm just saying that with sorting applied changes will have to be reviewed. The sort list perhaps won't always have groups like tsconfigs in individual packages.
I don't expect we'll have many such cases going foward.
Which part is slow? The sort or the compare? (Others are sort of fixed.) |
Haven't dug into it at that level but I think it's the sort. The compare is just a string equality check. But there's also looking up the prettier config and reformatting, which could be a big chunk of the time. I could try normalizing with a fixed config instead of looking it up, and then reformat using the config settings only if needed. But still I think the sort itself is the expensive part. |
54bc012
to
8cb61d9
Compare
I see that the sort is also prettification. I think the best option is probably to check if sorted rather than to sort and format and compare. Reading and checking order could be pretty fast. This could be custom here until sort-jsonc is updated with a new feature. :) |
To be clear, are you suggesting I re implement most of the sort? I'd prefer not to implement anything like that. IMO if it's not simple enough to plug something in to do it, it's not worth the dev and maintenance cost. That is, I don't want to maintain code that checks if tsconfigs are sorted (vs plugging in code maintained elsewhere to do it). If I were to implement this I think I would do it in sort-jsonc, adding a check option with an early exit in the sort. |
I opened duniul/sort-jsonc#6; might implement it myself if the maintainer is open to it. |
I was not suggesting implementation of the sort. It is one thing to check and another to sort. Checking if sorted is efficient with simple in-order walk. Actual sorting is not usually efficient walking in-order. Efficient sorting may also not lend its hand to an is sorted check add-on. Just my thoughts on the posed performance topic. |
4067fdf
to
c456cbd
Compare
This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework! |
Adds a new policy handler to check and sort tsconfig files based on an order we define. Comments in the tsconfig files should be maintained, and the resolver should output files formatted according to their prettier settings.
Problem: To check if a file is sorted, I load it and sort it, then compare it to the original (in a normalized way). This is slow. If you have ideas for speeding up the
isSorted
check let me know.I ran the new handler on the build-tools folder manually and included those changes here.