Skip to content

docs: typo #1480

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

Merged
merged 2 commits into from
Feb 28, 2025
Merged

docs: typo #1480

merged 2 commits into from
Feb 28, 2025

Conversation

MatousMarik
Copy link
Contributor

No description provided.

@MatousMarik MatousMarik requested a review from TC-MO as a code owner February 26, 2025 11:17
@@ -299,7 +299,7 @@ The SDK provides convenient methods for exiting Actors:

1. Use `exit()` with custom messages to inform users about the Actor's achievements or issues.

2. The `exit()` method emits `exit` event allowing components to perform cleanup or state persistence.
2. The `fail()` method emits `exit` event allowing components to perform cleanup or state persistence.
Copy link
Member

@B4nan B4nan Feb 26, 2025

Choose a reason for hiding this comment

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

the fail method calls the exit method. both of them will issue process.exit() unless explicitly told not to (or better say, the exit method is the one doing this, fail just calls it), the difference is fail defaults to exit code 1.

https://github.com/apify/apify-sdk-js/blob/master/packages/apify/src/actor.ts#L292-L294

(and both methods emit the exit event too, the only difference really is in the exit code)

Copy link
Contributor

@TC-MO TC-MO Feb 26, 2025

Choose a reason for hiding this comment

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

So we could change it to :

Suggested change
2. The `fail()` method emits `exit` event allowing components to perform cleanup or state persistence.
1. Use `exit()`/`fail()` with custom messages to inform users about the Actor's achievements or issues.
2. The `exit()`/`fail()` method emits `exit` event allowing components to perform cleanup or state persistence.

?

Or am I misunderstanding it?

Copy link
Member

Choose a reason for hiding this comment

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

i am not sure if we should have it written this way with the two bullet points. all the fail method is doing is calling exit with exit code 1. i think that's what we should say in the docs, so its clear its just a shortcut with an error exit code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Righ then maybe something like this, with bullets instead of ordered list

Suggested change
2. The `fail()` method emits `exit` event allowing components to perform cleanup or state persistence.
- Use `exit()` with custom messages to inform users about the Actor's achievements or issues.
- Use `fail()` as a shortcut for `exit()` when indicating an error. It defaults to an exit code of `1` and emits the `exit` event, allowing components to perform cleanup or state persistence.
- The `exit()` method also emits the `exit` event, enabling cleanup or state persistence.

Copy link
Member

Choose a reason for hiding this comment

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

yeah that's much better 👍

@TC-MO
Copy link
Contributor

TC-MO commented Feb 26, 2025

why are we still getting 401's? I thought the package issue has been solved?

@B4nan
Copy link
Member

B4nan commented Feb 26, 2025

Not really, we'll first have to update the use in here. And we cannot do that, because the latest version of the public packages are broken, they are set to be ESM, but they don't respect what that actually means (having extensions in all imports in particular), so it's not usable for us right now. I'll update the repository to use them once it's resolved.

@TC-MO
Copy link
Contributor

TC-MO commented Feb 26, 2025

ok makes sense, thank you!

add information about `fail()` method being shortcut for `exit()`
remake ordered list to bullet list
mention that both methods emit `exit` event
@TC-MO TC-MO requested a review from B4nan February 28, 2025 10:48
@TC-MO TC-MO merged commit 443a94b into apify:master Feb 28, 2025
3 of 6 checks passed
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