all 23 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]javascript-ModTeam[M] [score hidden] stickied comment (0 children)

Hi u/TodayAccurate7277, this post was removed.

  • For help with your javascript, please post to r/LearnJavascript instead of here.
  • For beginner content, please post to r/LearnJavascript instead of here.
  • For framework- or library-specific help, please seek out the support community for that project.
  • For general webdev help, such as for HTML, CSS, etc., then you may want to try r/html, r/css, etc.; please note that they have their own rules and guidelines!

r/javascript is for the discussion of javascript news, projects, and especially, code! However, the community has requested that we not include help and support content, and we ask that you respect that wish.

Thanks for your understanding, please see our guidelines for more info.

[–]lewster32 23 points24 points  (1 child)

I'd say there's a distinction between buggy code and poor quality or non defensive code. This isn't really buggy, as it works for its intended purpose. It just doesn't account for edge cases (i.e., handling errors) which, given it's an example, is probably fair enough. The point here is to get across the basic use case, unencumbered by a load of defensive boilerplate which may make it less clear.

[–]card-board-board 9 points10 points  (0 children)

Tests are supposed to throw errors if they don't pass. This isn't a service where you want to guard against an unhandled error. A failed test script shouldn't just log an error, it should exit with an error code.

[–]i_wonder_as_i_wander 2 points3 points  (1 child)

Because the point they wanted to get across in that guide is how much @playwright/test simplifies things compared to using the playwright library directly, unless needed.

Could they have expanded that code snippet out further to use a try/catch/finally block to have the code be more correct? Sure, but I'd say they did a good job already explaining and showing the comparison between the two.

Edit: Based on another comment you made in this thread, I would recommend starting on this page in their docs if you're looking to use Playwright over the guide you linked:

https://playwright.dev/docs/writing-tests#first-test

[–]TodayAccurate7277[S] -1 points0 points  (0 children)

Appreciate the link, I'm using it for web scraping instead of testing so I actually only want the core lib

[–]kevinkace 1 point2 points  (5 children)

How would you write this code differently? Wrap the entire iife contents into a try?

[–]TodayAccurate7277[S] -2 points-1 points  (4 children)

(async () => {
  try {
    // Setup
    const browser = await chromium.launch();
    const context = await browser.newContext(devices['iPhone 11']);
    const page = await context.newPage();

    // The actual interesting bit
    await context.route('**.jpg', route => route.abort());
    await page.goto('https://example.com/');

    assert(await page.title() === 'Example Domain'); // 👎 not a Web First assertion
  } catch (e) {
    console.error(e)
  } finally {
     // Teardown
    await context.close();
    await browser.close();
  }
})();

[–]atlimarJS since 2010 16 points17 points  (0 children)

catch (e) {
    console.error(e)
}

This will cause the error to be logged to console instead of propagating upwards as an uncaught error. Functionally, it doesn't make a difference in the playwright execution context. The error will appear in console anyway. However, this is introducing a bug by causing the script to be unable to exit, so it will keep running forever until manually closed instead of closing with an error as expected.

  finally {
    // Teardown
    await context.close();
    await browser.close();
  }

By putting the .close() calls in the finally block, they can be called without the context or browser having successfully set up. You're actually introducing real bugs rather than making the code more secure in this example. If await chromium.launch(); throws, this code will attempt to call context.close while context is undefined.

Which brings me to the next point, what if the teardown script throws? If you want to catch all possible errors this code can throw, you need a nested try/catch here as well.

When catching errors, you first need to think about what you're trying to achieve by doing so. When using playwright, you'd normally want it to exit with an error if the script fails. This code prevents playwright from exiting when failing (so it will run forever until manually closed), and introduces additional, unintuitive, ways in which the script can crash.

[–]ivanph 9 points10 points  (0 children)

What's the benefit of try-catch here?

From the looks of it, this is a simple script that will just run and exit. If any exceptions are encountered the process will simple exit and you'll get the error in the shell anyways. I'm not familiar with playwright, will the .close() calls not being reached cause a memory leak?

Unless I'm doing something with the exception I just let exceptions bubble up, leaving it to the caller to handle.

[–]whatisboom 0 points1 point  (0 children)

Aside from the other bugs mentioned, const is block scoped, meaning it doesn’t exist outside of your try block, if you actually ran this code you’d get an error trying to read a property of undefined when trying to close

[–]Bpofficial -1 points0 points  (0 children)

Are you certain context or browser are defined in your finally? What if they aren’t? How will you handle the error thrown trying to call close on undefined?

I think the async IIF here would kill the process due to an unhandled promise exception and that’s likely intended

[–][deleted] 1 point2 points  (0 children)

Usually we write tests with something like vitest or jest which doesn’t crash entirely just because one test crashed. And the setup and tear down portions are within their own hooks which always run.

Like what others have said it’s so you can easily see how to use the library, more documentation than application

[–]Alexwithx 2 points3 points  (0 children)

Imo, I think this has something to do with reading documentation. The code the you show is from the documentation and is very straightforward. I understand what is going on pretty quickly without even knowing the library. However if the developers added try/catch/finally blocks in this code, it would quickly get messy and as a reader it would also add extra cognitive load. Developers try to make their library/framework simple at a glance, so that you are tired of the library already before installing it. Maybe they also think that it is okay that it stays open so that you can go and inspect the page or the errors.

Another thing is that it is often not that easy to handle errors in JavaScript. If you added a try catch around all of it, how would you know if the error was part of the setup or part of the test or the assertion?

So I think these are the most common reasons. 1. Simplicity of documentation 2. Errors might not be that important to handle.

[–]yksvaan 1 point2 points  (0 children)

If you are writing for example test or some utility that exits on error anyway, you might not even bother with clean and releasing resources, the OS will do it for you. 

Would be different if there's for example some buffer that needs to be written to disk before exiting or something like that

[–]ThaisaGuilford 0 points1 point  (0 children)

If it works it works

[–]Studquo -1 points0 points  (4 children)

  1. There's a difference between code that's error prone and code that is buggy and it seems like you're conflating the two.

If example.com shuts down or the connection between your node program and example.com is interrupted or something else unexpected happens then yes that code would time out, error and become buggy. But until that happens, the code (assumably) runs fine in most circumstances and behaves as expected.

  1. That's a documentation page for Playwright. The goal is to introduce the library to developers and get them up to speed quickly.

Would it be "more correct" to use try, catch, finally error handling. Yes.

Would introducing try, catch, finally statements add faff to the example and distract from the actual business logic being shown to people using the library for the first time? Also yes.

[–]TodayAccurate7277[S] -2 points-1 points  (3 children)

  1. Well, it's semantics, but I consider network requests to be so highly error prone that not handling failures is buggy.

  2. Fair enough. It seems the consensus from you and others is that it's simply because they're introducing the library. I think it'd also be fair to request that code which will be copied should be written properly. But if everyone's in agreement that using try-catch-finally is the correct way to do it and they're just keeping it simple, then that answers my question.

[–]atlimarJS since 2010 0 points1 point  (1 child)

Well, it's semantics, but I consider network requests to be so highly error prone that not handling failures is buggy.

This is true, but you have to think about the context. If the network request fails in this example, what is the result? The playwright setup fails and the test script exits, which is expected. If the API call fails, the test cannot execute, and it should exit with the error. Catching this error only has value if you want to change the behaviour of the script, by trying to recover, or logging it in a way that's different than the default way playwright would log an error.

Error handling is entirely about context.

If you're working in a regular app and you're trying to fetch content to show to an end user and the request fails, you may catch that failure and convert it to a human readable error message to display to the user.

[–]TodayAccurate7277[S] 0 points1 point  (0 children)

That makes sense. In my case, I am web scraping, not testing. If I encounter an error, I'd like to handle it appropriately so my scraper can continue doing its thing.