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

Fix source build when using --no-build-js #59118

Merged
merged 8 commits into from
Nov 27, 2024
Merged

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Nov 22, 2024

  • Checks in blazor.webview.js
  • Updates Microsoft.AspNetCore.App.Internal.Assets to build successfully when using --no-build-js, even when the framework JS assets are not on disk

Fixes #59114

@MackinnonBuck MackinnonBuck requested a review from a team as a code owner November 22, 2024 19:28
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Nov 22, 2024
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

We only need --no-build-nodejs for source build, isn't that the case?

Can we not instead update the check so that in that situation just disable the check that triggers the error when we are running inside source build?

Source build doesn't run any code, it just builds it, and the JS assets are not needed for the build step.

@MackinnonBuck
Copy link
Member Author

MackinnonBuck commented Nov 22, 2024

We only need --no-build-nodejs for source build, isn't that the case?

Can we not instead update the check so that in that situation just disable the check that triggers the error when we are running inside source build?

Source build doesn't run any code, it just builds it, and the JS assets are not needed for the build step.

Yeah, that sounds good. I'll update this PR to disable the check if not building with NodeJS.

Eventually I think we'll want to change blazor.webview.js to also not be an embedded resource and instead be included as part of M.A.A.Internal.Assets

@MackinnonBuck MackinnonBuck changed the title Temporarily check in Blazor framework JS files Disable blazor.webview.js check when not building NodeJS Nov 22, 2024
@MackinnonBuck MackinnonBuck changed the title Disable blazor.webview.js check when not building NodeJS Fix source build when using --no-build-js Nov 22, 2024
@javiercn
Copy link
Member

@MackinnonBuck do you think instead we could do something like #59135 to avoid checking-in the webview.js file?

@wtgodbe
Copy link
Member

wtgodbe commented Nov 26, 2024

We only need --no-build-nodejs for source build, isn't that the case?

I disagree, it's also useful for local builds. By default, if node isn't installed, the build will function as if --no-build-nodejs was passed - that scenario should still work outside of source build.

@MackinnonBuck
Copy link
Member Author

We only need --no-build-nodejs for source build, isn't that the case?

I disagree, it's also useful for local builds. By default, if node isn't installed, the build will function as if --no-build-nodejs was passed - that scenario should still work outside of source build.

If we want this to work more than just the build passing when --no-build-nodejs is specified, my guess is we'd want to do the following:

  • For blazor.{web|server|webassembly}.js, fall back on the implicit package reference to Microsoft.AspNetCore.Internal.Assets brought in by the SDK. Note that when building the aspnetcore repo locally, the assets brought in by that package reference do have potential to be out of sync with those that would be produced from a local build.
  • For blazor.webview.js, we'd need to either continue checking in the built assets or take a similar approach to the other blazor.*.js assets.

@javiercn
Copy link
Member

We only need --no-build-nodejs for source build, isn't that the case?

I disagree, it's also useful for local builds. By default, if node isn't installed, the build will function as if --no-build-nodejs was passed - that scenario should still work outside of source build.

We can make the build not fail, but if you don't have nodejs installed it's impossible to produce a correct set of outputs if they aren't somehow pre-generated. The only reason why that worked in the past is because the JS files were checked-in, which we aren't going to go back to.

We can choose to skip building the webview project if that's what we have to do here.

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 27, 2024

We can make the build not fail, but if you don't have nodejs installed it's impossible to produce a correct set of outputs if they aren't somehow pre-generated. The only reason why that worked in the past is because the JS files were checked-in, which we aren't going to go back to.

If it's possible to avoid the node dependency in the product build, that would definitely be preferred from a product construction POV. AspNetCore is the only repository that needs the node dependency when building the .NET SDK product.

@javiercn
Copy link
Member

If it's possible to avoid the node dependency in the product build, that would definitely be preferred from a product construction POV. AspNetCore is the only repository that needs the node dependency when building the .NET SDK product.

With the changes that we recently made, nodejs is not required anymore to build the SDK. The Microsoft.AspNetCore.Components.WebView library is a NuGet package, same as the JS assets that will now be downloaded by the app at project build time. I'm fine if we make the repo skip building those two packages when node is not available or explicitly as part of source build.

However, I want to be very clear that our E2E require JavaScript and that there's no way the product can be successfully built without it. This is the exact same situation as it is for wasm, that ships dotnet.js in a similar way.

The only reason this worked in the past was because we manually checked-in the JS asset outputs from the compilation on every release, which is something we are explicitly trying to avoid as it has security implications and causes immense problems for any dev working on the codebase.

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 27, 2024

However, I want to be very clear that our E2E require JavaScript and that there's no way the product can be successfully built without it. This is the exact same situation as it is for wasm, that ships dotnet.js in a similar way.

Repo boundaries don't matter when we think about the VMR which builds aspnetcore as one of 30 different repositories. If one repository needs node to build all the expected outputs (including OOB nuget packages) then it's a required dependency. Sounds like node continues to be a design time component in the aspnetcore repo and therefore we need to provide it.

@akoeplinger
Copy link
Member

This is the exact same situation as it is for wasm, that ships dotnet.js in a similar way.

One thing that's different for wasm in dotnet/runtime is that we use our own NodeJS build as a nuget package, this works because we can depend on that for the Microsoft build and wasm isn't built in sourcebuild.

@javiercn
Copy link
Member

@akoeplinger the parts that we are discussing here aren't either.

They used to be, and that's the bit that we are changing. The discussion is about avoiding a build failure in this case even when no NodeJS version is installed on the machine.

We could certainly switch to bringing in a nodejs version here too, but I don't think we need it.

@javiercn
Copy link
Member

@MackinnonBuck I discussed this with @akoeplinger offline.

We will still need nodejs on the VMR to build the product (not just Blazor), so I don't think we need to do any further changes other than avoid breaking during source build.

@MackinnonBuck MackinnonBuck requested a review from a team as a code owner November 27, 2024 18:09
@MackinnonBuck
Copy link
Member Author

MackinnonBuck commented Nov 27, 2024

I'm also adding a step in the local validation check to ensure that the build succeeds with -NoBuildNodeJS

@MackinnonBuck MackinnonBuck merged commit 81a2bab into main Nov 27, 2024
27 checks passed
@MackinnonBuck MackinnonBuck deleted the mbuck/check-in-js branch November 27, 2024 22:14
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview1 milestone Nov 27, 2024
@danroth27 danroth27 added the task label Feb 11, 2025
captainsafia pushed a commit that referenced this pull request Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building the managed part without node installed doesn't work anymore
6 participants