Disable the Hydrate task on client only configs #58
Conversation
|
Hooray! All contributors have signed the CLA. |
|
Some initial thoughts here: Can we add a test for this - particularly with the hardcoded 'Hydrate'. These could potentially be better typed across the codebase as keys to facilitate this kind of behaviour in a smoother more typesafe way. Thanks for the contribution! |
|
Hi @DarkPurple141, sorry for my late reply and thanks for your feedback! Totally agree this would be good to test. I've covered this as part of the group filtering spec, I think this is where it makes the most sense. Let me know what you think! |
|
This is an interesting one for sure. A 'hydrate' task does not make sense when there is no 'server' component, as hydrate requires SSR to work. Hydrate seems to straddle the line between client and server. |
| @@ -31,7 +31,13 @@ export function getGroups({ | |||
| result.push(server); | |||
| } | |||
| if (allowedGroups.includes('client')) { | |||
| result.push(client); | |||
| const tasks = allowedGroups.includes('server') | |||
There was a problem hiding this comment.
This seems brittle to me - what if we changed the task name in the future?
There was a problem hiding this comment.
In order to allow hydrate both server and client needs to be in the allowed groups
There was a problem hiding this comment.
Oh I see, this condition is inside a block that is checking for client and server
Can we add a comment to the code explaining that hydrate can only be run if both client and server is allowed?
| @@ -15,7 +15,7 @@ import server from '../../src/tasks/preset/server'; | |||
| it('should allow filtering of tasks (client only)', () => { | |||
| // 1: Initial render | |||
| const channel: Channel = new Channel({ async: false }); | |||
| const { container, queryByText } = render( | |||
| const { container, queryByText, debug } = render( | |||
There was a problem hiding this comment.
I don't think debug is needed now
There was a problem hiding this comment.
Sorry about this, my editor probably forgot to warn me about it
|
Hydrate is not a client task: it requires 🤔 |
| @@ -31,7 +31,13 @@ export function getGroups({ | |||
| result.push(server); | |||
| } | |||
| if (allowedGroups.includes('client')) { | |||
| result.push(client); | |||
| const tasks = allowedGroups.includes('server') | |||
There was a problem hiding this comment.
In order to allow hydrate both server and client needs to be in the allowed groups
|
Perhaps a string match on the task name "hydrate" is the best we can do for now... |
|
I think the string matching on the task name is the best we can do at this stage without much larger changes. Only minor changes left then:
|
|
Thanks for your work on this @robinmetral!! ❤️ |
|
I've merged this PR and will raise another one to make these minor changes – thanks @robinmetral 🎉 |
…y-followup Follow-up changes from "Disable the Hydrate task on client only configs" (#58)
|
Thanks everyone, sorry I couldn't address the last small comments before the merge, the discussion happened in the middle of the night for me 🙈 Thank you for picking it up @AndrewOCC! Last small note, I think the new version 0.14 didn't get released to NPM, is this expected? Thanks again for the great tool 🚀 |
Closes #57
There are obviously several ways to do this. I chose not to do it in
preset/client.tsxbecause it would have required to export a function (taking an allowedGroups param) instead of an object (which is the current export in both the client and server presets).I think that filtering the tasks in get-groups is a good option because the allowedGroups is already in use there, but I realise that this introduces some extra complexity (it will break if we change the Hydrate task's name).
Happy to take any feedback!