NullVoxPopuli / ember-query-params-service

Do you have controllers that _only_ parse query params? now you can get rid of them :)
MIT License
57 stars 9 forks source link

[Feature Request] Fastboot Support #479

Open marcemira opened 1 year ago

marcemira commented 1 year ago

Hello @NullVoxPopuli, would there be any plans on supporting fastboot here? I wouldn't care forking and trying myself. Let me know. Thanks

NullVoxPopuli commented 1 year ago

I don't use fastboot myself, so I can't personally support it.

If someone were to submit a PR with tests, I'd be happy to accept that though.

What's needed to support fastboot?

marcemira commented 1 year ago

It's the usage of window here https://github.com/NullVoxPopuli/ember-query-params-service/blob/master/addon/services/query-params.ts that causes an error during fastboot rendering time, because it just doesn't exist when the app runs on node. There 2 specific things to do there, which can be solved by slightly altering the logic with the help of the fastboot service (https://ember-fastboot.com/docs/user-guide#the-fastboot-service) While on fastboot, one will avoid using window.history entirely (no need), and the other would provide what window.location would with request information (a correct parsing of the request url should do it). Let me try a PR with tests, will probably help a lot of folks also using fastboot to adopt this addon as well :)

NullVoxPopuli commented 1 year ago

is there a way to do this without adding fastboot as a dependency?

like, if (history in globalThis) checks or something?

marcemira commented 1 year ago

There is: https://ember-fastboot.com/docs/addon-author-guide, but partially. We could make it so all window things are conditional, if it exists. No need for fastboot deps there. But it would also make sites not render correctly during fastboot, because query params won't work (because there's no location). Let me work on a solution for that that doesn't involve adding the fastboot dependency, although it can be disabled / removed if the main app doesn't depend on it, by using ember-cli tree manipulations... let me explore that as well

gilles-bertrand commented 1 year ago

Hi any evolution for this specific topic ?

NullVoxPopuli commented 1 year ago

I probably need to do some repo-maintenance to get things up to date, and then for fastboot support, since I don't use fastboot, I'd hapily take a PR that does "feature checking" like in https://github.com/NullVoxPopuli/ember-query-params-service/issues/479#issuecomment-1224175981 -- the main thing I want to avoid is adding fastboot as a dependency

marcemira commented 1 year ago

We ended up just doing this: https://github.com/feldico/ember-query-params-service/commit/0c61016a85d64a807533bcd6b1261f361638a9de, which fixes the error you get while on fastboot time, and all qp are definitely there too, so everything works. Not sure if that's the best implementation, this just got us out of the problem.

AmauryD commented 1 year ago

Maybe we could add fastboot as an optionalDependencies. And check at runTime if the service is available ?

NullVoxPopuli commented 1 year ago

There's no need though?

NullVoxPopuli commented 11 months ago

I finally got around to do some general maintenance, and am willing to take PRs if anyone wants to have a go at this.

Also a new test-app would be good that sets up fastboot to ensure no regression