fix(vite): do not use merged options for preview's build target #14815
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
@vicb I merged this. I don't know what you think about this, let me know if you think it makes sense! :D |
|
Thanks @mandarini to ping me here! TL;DR I think this PR is wrong and should be reverted. Yet I think I understand where @dmitry-stepanenko comes from and the code might be improved. Details Why this is wrong ? First because some of the build options can be overridden in the preview options - namely So if we do not merge the options the overrides will not work. Second the merged options are computed as const mergedOptions = {
...{ watch: {} },
...buildTargetOptions,
...options,
};The purpose of What could be improved? With the following code: const mergedOptions = {
...{ watch: {} },
...buildTargetOptions,
...options,
};The As of today nothing prevents you from adding an options that is not in the Notes:
So the most correct way would be:
Is it worth the extra effort? I did notice this little inconsistency when working on the code but I didn't think it was worth "fixing". That being said I don't think there is any cons of adding this validation against the schema. It could make things a little cleaner and also could catch typos in the target configuration. However if we decide we want to validate against the I hope this make sense? |
|
Thank you @vicb ! @dmitry-stepanenko can you please add your comments here, too? Regarding the "options validation against schema.json", we don't want to do that. We want to allow the user to pass any options they may need, that are supported, for example in this case, by the Vite CLI, without us having to keep an exhaustive schema.json of ALL the available options, and maintain it as such. Same goes for other executors that pass down CLI options (eg. storybook, etc). That being said, what do you think would be the best way to improve our implementation in this case? Do you think there's something undocumented that we could document in a better/more understandable way? |
That is not supported today: nx/packages/vite/src/executors/build/build.impl.ts Lines 22 to 29 in e7f5855
|
|
Thanks @vicb. Unfortunately this PR is not aimed at simple code improvement, I'm addressing an issue that cannot be solved without these changes. The problem is this statement is not always true
The actual state of things is
I don't think that loosing an ability to control targets separately is correct. I have a scenario where I have to use the build target with |
That's exactly what I meant - we are aligned here.
If I understand correctly you want The promise of But I might miss something about your setup? |
|
Yes, my setup is not standard, vite executors are being used for the qwik nx wrapper. In native Qwik applications build command is invoked twice: for the client and server side setups. These commands should be invoked in the specified order. We're having the same behavior with 2 build targets: "build-ssr": {
"executor": "@nrwl/vite:build",
"options": {
"outputPath": "dist/apps/cart",
"ssr": "src/entry.preview.tsx"
},
"dependsOn": [ "build" ]
}In my case another option would be to combine "build" and "build-ssr" targets into one custom executor, but that will create other set of questions in terms of |
|
If I understand correctly:
Is that correct? (kind of hard to understand without the full config and without more details). |
|
I will look at the outcome of this conversation tomorrow, because it is late for me! Please do let me know how I can adjust our code to make things better, and please let me know if I will need to revert the commit, or push some changes! Thank you both!!!! |
|
For now I would revert this PR because it will affect everybody using the vite preview executor In parallel @dmitry-stepanenko should create an issue to discuss about his "not standard" setup and the best way to support that. Edit: Some more context: The idea of the vite/preview executor is to mirror what the CLI command |
|
Ok, @vicb @dmitry-stepanenko I am reverting. Let's see, in any case, how I can improve the whole options implementation, ok? |
|
@dmitry-stepanenko please reach out to me/jack on the Community Slack to see what we can do! |
|
@vicb @mandarini I understand Victor's point of having the same options for build and preview targets, as discussed above, this makes sense in most "standard" scenarios. However, I do believe that we should also be able to support use cases beyond this. So I would suggest adding a Here're the changes that I think should do the job, please let me know if this works for you guys https://github.com/nrwl/nx/compare/master...dmitry-stepanenko:ds/vite-preview-custom-build-target?expand=1 |
|
I will not have time to review this until tonight or tomorrow. If you can both wait, I can devote tomorrow understanding what's going on, because I haven't had time yet to look into the issue, and I have been relying on your suggestions only (which is fine, but I really should look into it myself too). So, yeah, I'll put a pause here, and will dive into this tomorrow, devote it the appropriate time, and come back to you both! Thank youuuuu!!! |
|
@vicb just some more context for you to understand my problem. The goal that I'm following is to go through these commands sequentially using @nrwl/vite executors:
You can see this in action here https://stackblitz.com/edit/qwik-starter You can also play around with a real app by running Using changes that I referenced above it is possible to configure it like this "preview": {
"executor": "@nrwl/vite:preview-server",
"options": {
"buildTarget": "myapp:build-ssr",
"customBuildTarget": true
},
"dependsOn": ["build"]
},This also opens a perspective for me to wrap 2 build commands into 1 custom executor at some point, if we decide to. |
|
@dmitry-stepanenko thanks for the details I think I have a better understanding of what you need now. The vite config in https://stackblitz.com/edit/qwik-starter is export default defineConfig(() => {
return {
plugins: [qwikCity(), qwikVite(), tsconfigPaths()],
preview: {
headers: {
'Cache-Control': 'public, max-age=600',
},
},
};
});The qwik plugins do some magic behind the scene while serving the site so you can not just use a file server as I suggested earlier. I think what you want to do is:
I think this can look like: "preview": {
"executor": "@nrwl/vite:preview-server",
"defaultConfiguration": "development",
"options": {
"buildTarget": "qapp:build",
"proxyConfig": "apps/qapp/proxy.conf.json"
"build": false,
},
"configurations": {
"development": {
"buildTarget": "qapp:build:development"
},
"production": {
"buildTarget": "qapp:build:production"
}
}
},
"qpreview": {
"executor": "nx:run-commands",
"options": {
"commands": [
"nx run qapp:build",
"nx run qapp:build-srr",
"nx run qapp:qpreview",
],
"parallel": false
}
}The only update to
In your case you do not want to build so you would set that option to Does this sounds good? |
|
Thanks @vicb. The setup I have now is pretty much the same to what you are suggesting with the only difference I'm relying on the "dependsOn" mechanism instead of an extra target. Your solution should definetely solve the issue that I'm facing. And it fits very well in the existing case where we want the preview server to have the same options as the build target.
Don't you think that this approach https://github.com/nrwl/nx/compare/master...dmitry-stepanenko:ds/vite-preview-custom-build-target?expand=1 should provide more flexibility? Yes, we will loose the implicit combining of build and preview options, so that everything should be configured manually using configurations, but I don't think it's a big deal in the scenario where setup itself is not standard |
|
The
By default the watch mode is enabled and any app change will trigger a rebuild. For you project (more generally when there is a need to customize the build step) we could have the preview step do only what You have a good point when you say If I understand your changes correctly when you set IMO both our solutions solve the issue but not in the cleanest way. So I am going to propose a 3rd solution:
This means that the default configuration would remains unchanged and the current When there is a custom build step, you would not provide the What do you think of that? Some more details: Today
That's why even if we don't want to build (step 2) we still need I think it's a good discussion and we are converging to a good solution. At this point I think it is safe to say that the current PR could be reverted. We can support your use case using a nicer solution. What do you think? |
|
btw @dmitry-stepanenko maybe it's late to ask but I am not sure to understand what problem your initial CL was fixing (by looking at code and the description). Could you expand a little bit on that? That is what was not working? |
|
Thank you @vicb. Your alternative 3rd solution is much more suitable for generic purposes. But to be honest, after implementing your solution I still think that mine with Mainly because of these reasons:
Anyway, your solution should still be very convenient and understandable, I'm looking forward to proceeding with any of these 2. Let me know if you have any suggestion with its implementation, I hope I've done it according to your vision. Probably at this point it is worth letting @mandarini to decide 🙂 So Katerina, please let me know which of these 2 solutions you like more and whether you want me to make any changes to the one you pick (if any, of course): |
So the entire problem is that I need to invoke the ssr build between the client one and the preview server. Due to the order of execution I'm only able to specify the "build-ssr" as a "buildTarget" for the preview executor. Because of this preview server inferred wrong options from the build target. And the whole thing was solved by preventing the build options to be passed into the preview server |
|
Thank you both for the detailed descriptions. I will revert the initial PR, and look at the two solutions suggested by @dmitry-stepanenko. I think that our |
|
Please don't rush something in without discussion. I have a last idea that
can be nice too. I'll explain shortly.
…On Thu, Feb 9, 2023, 03:01 Katerina Skroumpelou ***@***.***> wrote:
Thank you both for the detailed descriptions. I will revert the initial
PR, and look at the two solutions suggested by @dmitry-stepanenko
<https://github.com/dmitry-stepanenko>. I think that our preview-server
should be consistent with the way vite preview works, in any case. I will
discuss this with Jack, too, and come up with a solution by the end of the
day.
—
Reply to this email directly, view it on GitHub
<#14815 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB4X4RI6HTL3Y2O3HVUVFTWWTE7RANCNFSM6AAAAAAUSRKMCM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
No rush at all! I just reverted that PR to make sure the original intended functionality remains the same. I will work on this tomorrow in any case. Thank you both very much |
|
Long but super interesting discussion... Having a solution that works does not mean it's a good solution. The solution should be understandable and explainable so that one can build a mental model of how to use it. The mental model for
It does not fit well the vite flow which is:
The steps must be run in this order. You can not run 2 before 1. So what @dmitry-stepanenko is trying to do is to fold 2 and 3 into a How do we reconcile all of that? If we still think to When What do you think - It looks pretty clean to me. Change details We add one optional When the executor runs it check what executor the build step uses. If the build step uses If the build step uses any other executor it makes no sense to merge the options. The buildTarget uses its own option as defined in the |
|
This makes sense @vicb ! Described above is quite similar to what I've done in my solution with customBuildTarget option. I've made a few changes there now to enrich the functionality according to your thoughts. That's definitely what was missing in it and it turns out to be nice indeed. Here're the main points about the modified flow:
The only thing I'm not sure about is |
|
I don't think we need the Otherwise I'm +1 on your CL. |
|
But this again enforces me to switch to another executor to avoid merging of configs, since with This option is false by default and just brings some additional convenience with edge cases |
|
In my opinion we don't need the customBuildTarget - let the Nx wizards decide. In my opinion it's ok for Nx to solve 99% of the problems out of the box and leave aside the 1% custom configs especially when there are other way to do things. You have different options for not having to use
|
|
Ok, Victor, that's great we at least have an agreement on the overall implementation, thank you for your help and ideas! @mandarini I believe that we would want to move forward with this one master...dmitry-stepanenko:ds/vite-preview-custom-build-target, the only question is whether you would want to keep the |
|
Ok, thank you both!!!! I will move forward with this on Monday! :D It's great that a conclusion was reached that works for all! |
|
Regarding |
|
Sounds goos to me. @dmitry-stepanenko use case is very specific and IMO better addressed with a custom build/preview executor. If more use cases arise in the future we could still re-visit the decision. I think @dmitry-stepanenko should open a PR with his current code as his solution is a good starting point. |
|
Wow thank you both!!!! :D I just tested, and merged the updates!!!!! |
|
Thanks @mandarini! |
|
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
Right now options that are provided for the
previewtarget are being merged with build target ones and used for both build and preview targets.Expected Behavior
This might not be correct, because in some cases we do not want the same to be applied for both targets and this behavior is not straightforward.
Moreover, there's an existing mechanism for customization of a target using configurations, so instead of passing certain options directly to the preview target and expect them to be applied to the build one, we should use something like
buildTarget: "build:preview", where preview configuration of a build target would apply everything itself.Related Issue(s)
Fixes #