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

Ensure that the Application.start static method uses overridden class #603

Merged
merged 1 commit into from Nov 18, 2022

Conversation

lb-
Copy link
Contributor

@lb- lb- commented Nov 17, 2022

Notes

I opted for no documentation here - I think this behaviour would be expected and also aligns with any manual start usage with instantiation of the class yourself.

For example, the following two ways of starting the application will now be the same (with this PR merged).

A - Starting an application as per documentation

class MyApplication extends Application {}

const element = document.getElementById('main');
const application = MyApplication.start(element); // without this PR - the `application` will be an instance of the Stimulus `Application` and not `MyApplication`

B - Setting up the instance manually & running start

class MyApplication extends Application {}

const element = document.getElementById('main');
const application = new MyApplication(element);
application.start();

Test format

Note that I had to write my own test case class as src/tests/cases/application_test_case.ts does not use the documented Application.start() approach (due to other tests relying on a separate setup step for the application before starting).

This is similar to the way that the test file src/tests/modules/core/error_handler_tests.ts also sets up its own test case but without the double-registration of a Stimulus application. Happy to adjust if this is not the way we want to do this.

- When using const application = MyApplication.start() - it now will use the actual MyApplication class instead of the non-extended Application class
- Fixes hotwired#586
@lb-
Copy link
Contributor Author

lb- commented Nov 17, 2022

I do not think the error is due to this PR's changes (tests ran ok locally).

17 11 2022 21:36:45.021:ERROR [SaucelabsLauncher]: Error: 17 Nov 21:36:45 - couldn't connect to https://saucelabs.com/rest/v1/stimulus/tunnels: Post https://saucelabs.com/rest/v1/stimulus/tunnels: net/http: timeout awaiting response headers

@dhh dhh merged commit 88b2641 into hotwired:main Nov 18, 2022
@lb- lb- deleted the feature/586-allow-class-extends-on-stimulus branch November 18, 2022 08:32
@lb-
Copy link
Contributor Author

lb- commented Nov 18, 2022

Thanks @dhh

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

Successfully merging this pull request may close these issues.

Ability to extend the Stimulus Application class
2 participants