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

v0.2 wip #72

Merged
merged 35 commits into from
Sep 11, 2024
Merged

v0.2 wip #72

merged 35 commits into from
Sep 11, 2024

Conversation

rixo
Copy link
Collaborator

@rixo rixo commented Jun 27, 2023

No description provided.

*/
const edit = async (file, contents) => {
const updatedPromise = new Promise((resolve) => {
// FIXME?
Copy link
Member

Choose a reason for hiding this comment

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

fix what? that the code below doesn't include the filename in the resolve check anymore? that could lead to false positives if there are multiple edits in parallel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's it.

As a matter of fact, this doesn't affect currently existing tests and, even though I don't remember, there was a reason I removed it.

My take is go with this version until we actually have the need because of some new tests... Which might very well be never, since focus will now shift on Svelte 5.

Copy link
Member

@dominikg dominikg left a comment

Choose a reason for hiding this comment

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

didn't check all the tests for soundness and the syntax/DSL for defining tests takes a bit to get used to but +1 to it being less verbose than classic playwright tests.

github matrix and package.json look ok.

needs some changeset files to describe the changes done to the package (dropped support for nollup, node12 etcpp)

.gitignore Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@rixo rixo force-pushed the v0.2-wip branch 2 times, most recently from 4ac617a to 22769e8 Compare January 6, 2024 22:33
@rixo rixo marked this pull request as ready for review September 10, 2024 22:30
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

awesome!!

eslint 9 is out now and is another potential upgrade that could be done, but I wouldn't block on it. it's also possible that some of the pnpm overrides are no longer necessary with the latest versions of all packages

@rixo
Copy link
Collaborator Author

rixo commented Sep 11, 2024

@benmccann

it's also possible that some of the pnpm overrides are no longer necessary with the latest versions of all packages

Indeed... I've removed them.

eslint 9 is out now and is another potential upgrade that could be done, but I wouldn't block on it

I gave it a shot, but it's not playing well with pnpm and the current config so, yeah, I'm leaving out of this MR.

@benmccann benmccann merged commit 7a9dd75 into master Sep 11, 2024
9 checks passed
@benmccann benmccann deleted the v0.2-wip branch September 11, 2024 10:16
@github-actions github-actions bot mentioned this pull request Sep 11, 2024
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.

3 participants