Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: implement test in memory execution based on TS aliases #17161
chore: implement test in memory execution based on TS aliases #17161
Changes from all commits
5fbf653
4bdcb0b
7fe552c
aed5f8d
3c8c268
20cf224
d2570b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why not include tsx in the transform by default, since it's so commonly used? Doesn't seem like it should hurt anything to have it listed here in packages that don't use tsx. (In general, if there are changes like this that we can make to reduce the number of manual overrides per package, I would very much prefer that in the interest of reducing maintenance headaches of having config duplicated tons of places.)
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.
to mitigate anyone to create
*.test.tsx
test files by "accident" for non react packagesThere 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 see the point, but how bad is that? To me it's not great, but I'd prefer the relatively low risk (and low consequence) of that occurring over the maintenance headaches that come from keeping a duplicated config in a whole bunch of places. (Not blocking on this though.)
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 see your concern. this is easily mitigated with existing oss tooling there is almost none, as everything can be updated/tweaked by proper migration/update scripts
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.
Not very important but why the spread?
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.
extensibility without less git diff noise.
my thinking was to keep it like this -> extract common preset to separate package with tests etc (not within scripts/), then just use api from that package and leave room for any tweaks additions in here without noisy git diffs
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.
Interesting, by "common preset" do you mean just the tsconfig or all shared elements?
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.
↓
or
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.
Since
@fluentui/utilities
no longer needs to be included in converged compilation, I'd prefer to revert these settings and the related changes.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.
Im gonna keep it. I don't see reason to throw out work already done, as this API change will cause no harm. Looks like until now consumers of that API had
any
all over the place anyway, this will be just explicit. WDYT?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 don't think that we should throw out any work, but at the same it's not anymore required for this PR 🤔
I propose to apply this changes in a separate PR (really want to have packages that have "strict" enabled ❤️ ). @ecraig12345 @Hotell what do you think?
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.
The main reason I was proposing reverting for now was in favor of a more detailed pass through to enable the option later (if we decide to invest in that). This would involve looking through the files and where possible, trying to understand the correct typing based on usage (and fix any missing null checks etc), rather than making the quickest change to silence the error.
I also very very strongly prefer to avoid
@ts-ignore
whenever possible since it's a "nuclear option": it turns off all type checking for the next line, rather than just the part where the error is coming from, opening up to more errors in the future.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.
you already spent time reviewing those changes, there are only 2 ts-ignores documented ( Im gonna create issue and ref it there as well) which don't affect any public facing API.
I explicitly checked those
!
non constructor assignment in affected files, to verify if that's good enough solution for now.To summarise, while I can create another PR , I don't think it's necessary (reasoning provided). As a side note/thought, this PR is open for almost 2 weeks which is blocking the whole improved DX initiative for convergence ✌️