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: up tsconfig target for more modern syntax #1780

Merged
merged 6 commits into from
Jan 15, 2024
Merged

Conversation

thepassle
Copy link
Contributor

No description provided.

@thepassle thepassle mentioned this pull request Oct 20, 2023
34 tasks
@thepassle
Copy link
Contributor Author

Ok well I expected this to be a more impactful change, but it seems everything seems to be working just fine :) gave this a quick spin on our example app and everything works, tested on ff/safari/chrome, tested on node 18, didnt run into any issues, so I think we can go ahead and merge this if you agree.

output looks a lot better, and will make debugging a lot easier :)

@kettanaito kettanaito added this to the Post-2.0 milestone Oct 22, 2023
Base automatically changed from feat/standard-api to main October 23, 2023 07:45
tsconfig.json Outdated
@@ -1,7 +1,7 @@
{
"compilerOptions": {
"strict": true,
"target": "es6",
"target": "ESNext",
Copy link
Member

Choose a reason for hiding this comment

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

I recall some tooling, I think it was Cypress, that has trouble consuming modern JavaScript. They require third-parties to be CommonJS. @thepassle, should that be our concern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds a little odd to me. A browser launcher expects code to be cjs? 🫠

@kettanaito
Copy link
Member

I've updated the branch. The change itself looks great, just need to make sure we aren't breaking third parties that cannot digest modern JavaScript. @thepassle, may I ask you to give this WIP build a try in our Examples repo? If all those pass, then we have nothing to worry about,

@thepassle
Copy link
Contributor Author

I've updated the branch. The change itself looks great, just need to make sure we aren't breaking third parties that cannot digest modern JavaScript. @thepassle, may I ask you to give this WIP build a try in our Examples repo? If all those pass, then we have nothing to worry about,

Is the Examples repo main branch up to date? I cloned it and pnpm installed, but im getting failed tests without making any changes:

image

@kettanaito
Copy link
Member

@thepassle, the examples repo needed the upgrade to msw@2.0. Just merged that upgrade, the tests are passing. You can give this build a try there.

I will also spend some time bringing back smoke tests but will make them on-demand (they are way too expensive to be run in CI on each change but it's a good idea to run them on changes where we expect the unexpected, like this one).

@thepassle
Copy link
Contributor Author

Alright cool, thanks. I'll give this another try today

@kettanaito
Copy link
Member

I've merged the smoke test workflow (as well as the bash script for local runs) so we should be able to run any PR against the examples repo easier now. I will update this branch and give it a try.

@thepassle
Copy link
Contributor Author

I've merged the smoke test workflow (as well as the bash script for local runs) so we should be able to run any PR against the examples repo easier now. I will update this branch and give it a try.

cool, thanks, let me know if theres anything I can help with :)

@kettanaito
Copy link
Member

@thepassle, do you happen to know how to allow the same GitHub workflow to run with both workflow_run and workflow_dispatch hooks?

I would like for the smoke-test workflow to trigger in two situations:

  • If the ci job completes successfully on main.
  • If it has been manually started (workflow_dispatch).

I have the following config:

on:
  # Always run smoke tests upon a successful
  # "ci" job completion on "main".
  workflow_run:
    workflows: ['ci']
    branches: [main]
    types: [completed]
  workflow_dispatch:

jobs:
  examples:
    if: ${{ github.event.workflow_run.conclusion == 'success' || github.event.workflow_dispatch }}

But GitHub doesn't seem to evaluate the jobs.examples.if expression correctly. Perhaps I'm writing it wrong.

@thepassle
Copy link
Contributor Author

I don't have a whole bunch of experience with github actions, but this is what github copilot tells me 😛

The github.event.workflow_dispatch event does not have a conclusion property. The conclusion property is only available for the workflow_run event.

If you want to run the job when the workflow_run event's conclusion is success or when the workflow_dispatch event is triggered, you can use the github.event_name property to check which event triggered the workflow. Here's an updated version of your code:

on:
  workflow_run:
    workflows: ['ci']
    branches: [main]
    types: [completed]
  workflow_dispatch:

jobs:
  examples:
    if: ${{ (github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success') || github.event_name == 'workflow_dispatch' }}

This will run the job when the workflow_run event's conclusion is success or when the workflow_dispatch event is triggered.

@kettanaito
Copy link
Member

@thepassle, thanks! That seems to be working (at least it doesn't discard the manual runs). Started a smoke test on this branch: https://github.com/mswjs/msw/actions/runs/6730818633

@thepassle
Copy link
Contributor Author

Hehe nice

Fingers crossed 🙂

@kettanaito
Copy link
Member

The smoke tests are passing. I deem this change worthy. Let's add it to the next minor version bump.

@thepassle
Copy link
Contributor Author

Looking good
image

kettanaito
kettanaito previously approved these changes Nov 2, 2023
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

Will schedule this in the next minor version.

@kettanaito
Copy link
Member

Updated the examples to use 2.0.14. Re-running the smoke tests against these changes and then merging.

@kettanaito
Copy link
Member

Alas, the smoke tests are broken :( Passes locally, passes in the Examples repo, fails miserably in the smoke-test job. Has to do with Playwright, only PW-based examples fail. Tried installing PW browsers literally at any place I could, no help.

@kettanaito kettanaito merged commit 29182ce into main Jan 15, 2024
11 checks passed
@kettanaito kettanaito deleted the feat/ts-config-target branch January 15, 2024 11:33
@kettanaito
Copy link
Member

Released: v2.1.0 🎉

This has been released in v2.1.0!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants