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

Set window.Turbo on import #280

Merged
merged 2 commits into from
Jun 8, 2021
Merged

Set window.Turbo on import #280

merged 2 commits into from
Jun 8, 2021

Conversation

excid3
Copy link
Contributor

@excid3 excid3 commented Jun 4, 2021

This makes sure Turbo is available for Request.js and Turbo Native out
of the box.

Related to rails/request.js#6

This makes sure Turbo is available for Request.js and Turbo Native out
of the box.
@excid3 excid3 marked this pull request as ready for review June 7, 2021 21:08
@excid3
Copy link
Contributor Author

excid3 commented Jun 7, 2021

Added a test to make sure the ESM module also sets window.Turbo

This one should be ready to go @dhh @marcelolx

@michelson
Copy link
Contributor

michelson commented Jun 7, 2021

are you sure that this a good idea @excid3 ?
what happens if a user does not want to expose this to window ? wouldn't be better to delegate this choice to the user ?

Copy link

@marcelolx marcelolx left a comment

Choose a reason for hiding this comment

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

Tested here and is working as expected.

@marcelolx
Copy link

are you sure that this a good idea @excid3 ?
what happens if a user does not want to expose this to window ? wouldn't be better to delegate this choice to the user ?

@michelson Have you a scenario where it would be a bad idea Turbo exposing itself to the window? I'm asking because I really don't know if there would be a problem or not

@dhh
Copy link
Member

dhh commented Jun 8, 2021

I think this is indeed the better default, but I'd also like to see an option where someone could import Turbo without triggering window + autostart. That would be a separate build target, though. That's more of a nice-to-have.

Having window.Turbo set is needed for all the native integrations anyway. And it makes it easier to drop Turbo into a site and start using it right away.

@dhh dhh merged commit 430cd50 into hotwired:main Jun 8, 2021
@michelson
Copy link
Contributor

michelson commented Jun 8, 2021

@michelson Have you a scenario where it would be a bad idea Turbo exposing itself to the window? I'm asking because I really don't know if there would be a problem or not

the only scenario I can imagine right now is for example some sort of widget app that could be imported on sites (like chat widgets like intercom) I would expect that app will self contained and not mess with the rest of the app.

weaverryan added a commit to symfony/ux that referenced this pull request Jul 5, 2022
…e-7)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Turbo] Fix TypeScript compilation issue... again

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| License       | MIT

_webpack.config.js_:
```js
Encore
    ...
    .enableTypeScriptLoader()
    .enableForkedTypeScriptTypesChecking()
```

Error message on `yarn encore production`:
```
Failed to compile with 1 errors

 error  in vendor/symfony/ux-turbo/Resources/assets/src/turbo_controller.ts:14:8                                                                                                    11:22:22

TS2339: Property 'Turbo' does not exist on type 'Window & typeof globalThis'.
    12 |
    13 | // Expose Turbo to the rest of the app to allow for dynamic Turbo calls
  > 14 | window.Turbo = Turbo;
       |        ^^^^^
    15 |
    16 | /**
    17 |  * Empty Stimulus controller only used for Symfony Flex wiring.
```

Previously, #331 proposed a different solution, but it was rejected due to the changes already being prepared in hotwired/turbo#280. At the moment, the changes in the second package have been made, as a result of which it is possible to change lines that cause a compilation error.

Commits
-------

25482a7 [Turbo] Fix TypeScript compilation issue... again
jameswebapp added a commit to jameswebapp/ux that referenced this pull request Aug 1, 2023
…e-7)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Turbo] Fix TypeScript compilation issue... again

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| License       | MIT

_webpack.config.js_:
```js
Encore
    ...
    .enableTypeScriptLoader()
    .enableForkedTypeScriptTypesChecking()
```

Error message on `yarn encore production`:
```
Failed to compile with 1 errors

 error  in vendor/symfony/ux-turbo/Resources/assets/src/turbo_controller.ts:14:8                                                                                                    11:22:22

TS2339: Property 'Turbo' does not exist on type 'Window & typeof globalThis'.
    12 |
    13 | // Expose Turbo to the rest of the app to allow for dynamic Turbo calls
  > 14 | window.Turbo = Turbo;
       |        ^^^^^
    15 |
    16 | /**
    17 |  * Empty Stimulus controller only used for Symfony Flex wiring.
```

Previously, #331 proposed a different solution, but it was rejected due to the changes already being prepared in hotwired/turbo#280. At the moment, the changes in the second package have been made, as a result of which it is possible to change lines that cause a compilation error.

Commits
-------

25482a7 [Turbo] Fix TypeScript compilation issue... again
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