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

OPTIONS http method Added #6030

Closed
wants to merge 2 commits into from
Closed

OPTIONS http method Added #6030

wants to merge 2 commits into from

Conversation

amunim
Copy link

@amunim amunim commented Aug 18, 2022

Fixed #5193

Formatted using pnpm format

  • [ issue Method Options - HTTP #5193 ]
  • [ server.test.js has a test] Ideally, include a test that fails without this PR but passes with it.
  • [I ran the tests] Run the tests with pnpm test and lint the project with pnpm lint and pnpm check
  • [no such changes ] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

Fixed sveltejs#5193

Formatted using `pnpm format`
@changeset-bot
Copy link

changeset-bot bot commented Aug 18, 2022

⚠️ No Changeset found

Latest commit: d155d71

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dummdidumm
Copy link
Member

Thank you! I'd like to hold off on merging this until we decided what to do with the form proposal, which likely changes what's allowed inside +page.server.js.

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.

Your new test seems to be failing:

@sveltejs/kit:test: 1 failed
@sveltejs/kit:test: [chromium-dev-no-js] › test/server.test.js:52:2 › Endpoints › OPTIONS request ==================

@rmunn
Copy link
Contributor

rmunn commented Aug 19, 2022

Nice work so far. As this seems to be your first contribution, I'd like to give a small pointer on the use of the PR template: those [ ] boxes are Markdown code for checkboxes, and you can put a check in them by writing [x] (convert the space to an x — I've seen [ x] and [x ] a lot, which won't work, it has to be [x]). What you've written in the PR description is fine, it's perfectly understandable, it's just not quite how the template was intended to be filled out. :-)

@benmccann
Copy link
Member

@amunim this PR would need to be rebased and the failing tests would need to be fixed

@amunim
Copy link
Author

amunim commented Sep 18, 2022

I am so confused at this point, I have literally searched the whole project and replaced all 204s with 555, 553, and 554 respectively but am still getting 204 as result in my tests.

@amunim
Copy link
Author

amunim commented Sep 21, 2022

I'll rebase and work on another issue for now...

@amunim amunim closed this Sep 21, 2022
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.

Method Options - HTTP
4 participants