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

Introduce Turbo.config object #1217

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Conversation

seanpdoyle
Copy link
Contributor

The initial implementation aims to replace the ad hoc configurations spread across window.Turbo and Turbo.session. Those objects shouldn't be considered part of the public interface, especially for extension.

This commit exports a single object from src/core/config to serve as a centralized, user-facing repository for configuration values.

The main src/core/config object is composed of two constituent parts:

  • src/core/config/drive
  • src/core/config/forms

In the future, the directory structure can be expanded upon to suit the needs of the project.

Any user-facing configuration outside of Turbo.config is deprecated.

@seanpdoyle
Copy link
Contributor Author

The initial implementation aims to replace the ad hoc configurations
spread across `window.Turbo` and `Turbo.session`. Those objects
shouldn't be considered part of the public interface, especially for
extension.

This commit exports a single object from `src/core/config` to serve as a
centralized, user-facing repository for configuration values.

The main `src/core/config` object is composed of two constituent parts:

* `src/core/config/drive`
* `src/core/config/forms`

In the future, the directory structure can be expanded upon to suit the
needs of the project.

Any user-facing configuration outside of `Turbo.config` is deprecated.
@seanpdoyle
Copy link
Contributor Author

@jorgemanrubia @brunoprietog are either of you able to review this proposal?

@OutlawAndy
Copy link

Hey @seanpdoyle. I was kind of surprised to learn of a change to Turbo's public API in a patch release. But seeing as that ship has sailed, I have another question for you.

We maintain a package that integrates with Turbo.setConfirmMethod and wondering if you have an opinion on the best way to handle this change?

I'd rather not introduce branching logic based on the existence of a Turbo.config object. e.g.

  if (window.Turbo.config) {
    window.Turbo.config.forms.confirm = confirmationHandler
  } else {
    window.Turbo.setConfirmMethod(confirmationHandler)
  }

However, I'd also prefer not to entirely drop support for previous versions of Turbo. Especially considering that I'm not offering any other reasons for upgrading our package other than you can't, unless you also upgrade Turbo itself.

I'm concerned that adding a peerDependency requirement only emits a warning that is easily overlooked, but will actually crash our package if left unmet.

I would appreciate any feedback you have on the matter.

Thank you,

  • Andy

our internal discussion: RoleModel/turbo-confirm#31

@seanpdoyle
Copy link
Contributor Author

@OutlawAndy thank you for sharing those details here. Personally, I am not involved in Turbo's release schedule, so I cannot provide any insights into that process. I will say that the deprecations in this diff were intended to buy time and provide hints to consuming packages.

This diff proposed the Turbo.config object to serve as the interface for configuring Turbo. Until the setConfirmMethod is removed, you can continue to call it instead of interacting with Turbo.config, but it is will emit a deprecation warning.

As far as the impacts to your package are concerned, I understand your distaste for adding a conditional to cover this case. However, if you are committed to maintaining support for all historical Turbo version, that decision involves supporting Turbo's full range of interfaces as they evolve. Turbo does not programmatically expose version information to the JavaScript runtime, so packages are left with graceful degradation-style conditionals to serve as safety nets.

Again, while I cannot speak to Turbo's release schedule or maintenance policies, I do believe that as the package and its interfaces evolve, there will be an evolving minimal threshold version that will serve as a baseline of backwards compatibility. For now, the deprecations keep Turbo.setConfirmMethod on the supported side of that threshold, but eventually that won't be true anymore.

I'm concerned that adding a peerDependency requirement only emits a warning that is easily overlooked

I think this ultimately boils down to your package's compatibility goals, but a peerDependency requirement feels one of the few concrete ways to encode your support goals to the consumers.

@OutlawAndy
Copy link

@seanpdoyle Thank you for the insightful response! That's quite helpful!

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