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

Add ability to set afterLoad static methods on Controllers #579

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

lb-
Copy link
Contributor

@lb- lb- commented Aug 30, 2022

Note: Hopefully I have added things correctly, I could not get the examples working so have only added unit tests.

@rik
Copy link
Contributor

rik commented Aug 30, 2022

afterLoad makes me think this will be called after the load event. afterRegister seems less confusing to me.

@lb-
Copy link
Contributor Author

lb- commented Aug 30, 2022

afterLoad makes me think this will be called after the load event. afterRegister seems less confusing to me.

Yeah - see #574 (comment)

Basically, the current static method shouldLoad is already in place.

While afterRegister better aligns with the overall nomenclature of register but then there will be two static methods; shouldLoad and afterRegister that will be inconsistent.

A good thing to note is that you can also load a suite of controllers via application.load([...someControllersWithIdentifiers]) - I think that is why the naming is inconsistent.

application.register only takes one controller (and calls application.load under the hood), application.load takes one or more controllers.

If we opt for afteRegister, I think we should also move to shouldRegister with handling for a legacy fallback of shouldLoad (although, this is a bit tricky due to the boolean check and the fact it is a static get method, so checking for its presence needs to be done carefully).

@marcoroth
Copy link
Member

marcoroth commented Sep 2, 2022

afterLoad makes me think this will be called after the load event. afterRegister seems less confusing to me.

What if we call it afterApplicationLoad()?

But yeah, agreed. The naming is not ideal. I wonder if it's worth to go through a deprecation cycle to align it to register.

@lb-
Copy link
Contributor Author

lb- commented Sep 2, 2022

I think afterApplicationLoad could be confusing in a different way. It may be inferred to be after the application has loaded instead of after this controller is loaded by the application.

What if I Update to afterRegister or afterRegistered ?

Then can explore a rename if shouldLoad to should register as a seperate issue.

@marcoroth
Copy link
Member

Looking at this function I can totally understand why it's called shouldLoad, it's just super confusing because you usually don't call load() in your application

load(...definitions: Definition[]): void
load(definitions: Definition[]): void
load(head: Definition | Definition[], ...rest: Definition[]) {
const definitions = Array.isArray(head) ? head : [head, ...rest]
definitions.forEach(definition => {
if ((definition.controllerConstructor as any).shouldLoad) {
this.router.loadDefinition(definition)
}
})
}

@lb-
Copy link
Contributor Author

lb- commented Sep 4, 2022

The documentation does reference application.load when used with the webpack helpers.

https://stimulus.hotwired.dev/handbook/installing#using-webpack-helpers

@lb-
Copy link
Contributor Author

lb- commented Sep 30, 2022

Note to self - rebase on master now that eslint/prettier is adopted.

- when a controller is registered, the `afterLoad` static method, if present, will be called
- it gets passed the application instance and the identifier that was used to register it
- resolves hotwired#574
@lb- lb- force-pushed the feature/574-after-load-static-method branch from 8deda6d to d5b3a53 Compare October 3, 2022 09:27
@dhh
Copy link
Member

dhh commented Nov 17, 2022

Went with shouldLoad because it's called as part of load. Even though the user invocation happens through register. I think @lb- is right to keep this consistent. Also don't think there's enough value in a rename to proceed with a depreciation cycle here to be honest.

@dhh dhh merged commit 2fdac1a into hotwired:main Nov 17, 2022
@lb-
Copy link
Contributor Author

lb- commented Nov 17, 2022

Thanks @dhh & @marcoroth

@lb- lb- deleted the feature/574-after-load-static-method branch November 17, 2022 10:55
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.

4 participants