Skip to content
This repository has been archived by the owner on Jul 17, 2020. It is now read-only.

Don't include the co-design sources directly. #46

Closed
wants to merge 1 commit into from
Closed

Don't include the co-design sources directly. #46

wants to merge 1 commit into from

Conversation

asynts
Copy link
Contributor

@asynts asynts commented Jan 30, 2020

Fixes #45

Currently, a copy of co-design resides in src/WebUI/wwwroot/lib/co-design. This pull request deletes these files and references the @codidact/co-design npm package instead.

The build instructions will get out of sync with the sources should this pull request be merged. In my opinion #31 should be merged first, and then I could adopt the build instructions to include a step "Install Node.js (for NPM)".

@luap42
Copy link
Member

luap42 commented Jan 30, 2020

Looks good, although I don't know whether it might be better not to include a version number, so that always the latest version is loaded. This might be a horrible idea, but it would prevent us from having to update the number with every update.

@asynts
Copy link
Contributor Author

asynts commented Jan 30, 2020

@luap42 I originally had the same idea, but it would get problematic in the following scenario:

  • There is some sort of flaw in co-design.
  • The issue is fixed but breaks compatibility.
  • A new version is released.

This would temporarily break the build and would make it impossible to go back in history and e.g. track down a bug.

@ranolfi
Copy link
Member

ranolfi commented Jan 31, 2020

I'm with @asynts in this regard. The explicit version number is a good thing, even though the dependency is in pre-1.0 release and under active development.

@ranolfi ranolfi requested a review from atlv24 January 31, 2020 05:23
@ranolfi
Copy link
Member

ranolfi commented Jan 31, 2020

For transparency's sake, I admit to still being reluctant to introduce a node.js and npm build dependency for a project we control.

For that reason I'm requesting input from contributors that I know are more familiar with .NET _Core_ projects than I am.

@@ -1,24 +1,31 @@
<Project Sdk="Microsoft.NET.Sdk.Web">
<PropertyGroup>
<TargetFramework>netcoreapp3.1</TargetFramework>
<RootNamespace>Codidact.WebUI</RootNamespace>
Copy link
Member

@DoctaJonez DoctaJonez Jan 31, 2020

Choose a reason for hiding this comment

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

I understand this is larger than the scope of this pull request, but the naming here doesn't follow the recommended pattern of Company.Technology.*. In this case it should be Codidact.Web.UI.

This should be addressed in the coding standards when they get published and ratified. We should fix this as early on in the project as possible, as addressing it later will be more difficult.

I assume there are more problematic namespaces elsewhere in the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should fix this as early on in the project as possible, as addressing it later will be more difficult.

I agree, we should address this very quickly. It might already be difficult because we have a few large pull requests.

I'll look for some other locations and create an issue about it.

Copy link
Contributor Author

@asynts asynts Jan 31, 2020

Choose a reason for hiding this comment

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

@DoctaJonez I was going through the sources and must say, I am conflicted about whether this is a good idea or not.

Your explicit suggestion doesn't really fit the schema either, because Web isn't really a technology that we develop. I'd say we are creating an QA product, thus Codidact.QA.WebServer would be correct, or something of the sort.

If this schema were applied rigorously it could look like this:

Codidact.QA.WebServer
Codidact.QA.Application
Codidact.QA.Domain
Codidact.QA.Infrastructure
Codidact.QA.AuthServer

This looks better than before but only marginally. Unless there is another big plus that I missed, this doesn't seem to be worth the trouble. (I also renamed WebUI to WebServer and Auth to AuthServer.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Your explicit suggestion doesn't really fit the schema either, because Web isn't really a technology that we develop. I'd say we are creating an QA product, thus Codidact.QA.WebServer would be correct, or something of the sort.

WebServer is the wrong word because its not a server for the web. WebUI refers to it being a presentation layer, as in displays stuff in the web.

Also the other projects will be sharing the infrastructure, domain and application projects so when we have an API for instance it will access these projects. So you can't really relate these projects to only QA.

I like this answer personally: https://stackoverflow.com/a/3929357/2490286

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you consider e.g. Infrastructure a project standing on its own? I am confused now.

If we were to add say a blog at codidact.com/blog that is completely independent from the question/answer thing, would these projects have the same infrastructure?

Copy link
Contributor

Choose a reason for hiding this comment

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

So you consider e.g. Infrastructure a project standing on its own? I am confused now.

If we were to add say a blog at codidact.com/blog that is completely independent from the question/answer thing, would these projects have the same infrastructure?

Actually giving it a lot of though these past 10 minutes I agree maybe we should add .QA after the Codidact since if we add a solution like blogging it will want to have its own infrastructure and domain.

Copy link
Contributor Author

@asynts asynts Jan 31, 2020

Choose a reason for hiding this comment

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

We might want to choose a different name though, there was some discussion somewhere about other types of posts, not just questions. I can't think of an umbrella term for it.

Addendum. On the other hand the landing page says "The Open Source Q&A Platform."

Copy link
Member

@DoctaJonez DoctaJonez Feb 3, 2020

Choose a reason for hiding this comment

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

Some good thoughts here.

I agree we need to choose an appropriate product name, which would leave the namespace as Codidact.ProductName.*

I had the same thoughts about Codidact.Web.* but for other reasons. Codidact.Web is a valid namespace that we might want to use at some point. Whenever we make utilities or extensions for various Microsoft namespaces, the convention is to mirror Microsoft's namespace, but using our own organisation name. So we may end up with libraries along the lines of:

  • Codidact.Web
  • Codidact.AspNet.WebApi
  • Codidact.Net.Http

I'm not saying we'd definitely need to implement these, just that it would be an appropriate convention, and we don't want to muddle these kinds of libraries with application specific code.

These theoretical libraries could also be exposed as public nuget packages if we feel there's anything useful in there that's worth making available for general consumption.

@DoctaJonez
Copy link
Member

DoctaJonez commented Jan 31, 2020

I don't know whether it might be better not to include a version number, so that always the latest version is loaded

It's very important for dependencies to be locked in for a specific release, and for that release to be re-producible at a later date. This will be invaluable when breaking changes are made to co-design, but we need to push out a hotfix for a live version that is using an older version of co-design.

@raphaelschmitz00
Copy link

For transparency's sake, I admit to still being reluctant to introduce a node.js and npm build dependency for a project we control.

For that reason I'm requesting input from contributors that I know are more familiar with .NET Core projects than I am.

I can't provide much input here except one thing: npm builds are slow.

At work I managed to reduce running time for a pipeline for C# unit tests from 15 minutes to 4 by skipping vue (= npm) and TypeScript builds. I also noticed with a smaller hobby project where I removed vue that my develop builds ran considerably faster.

That doesn't make npm a no go, but neither is it something to be underestimated: If a build takes 10 seconds, you continue working immediately; if it takes 2 minutes, you start checking your email etc and you actually lose way more than 2 minutes.

@ranolfi
Copy link
Member

ranolfi commented Feb 1, 2020

I checked out this PR and sure enough:

Error MSB3073 The command "npm install" exited with code 9009. WebUI

I'll be willing to merge this provided no one comes up with an alternative that doesn't require me to install node and npm, at least not manually and globally (at the machine level).

Each build dependency we add is an additional barrier for contributors and this is my primary concern.

So, what alternatives do we have? Any other sort of package manager that can be managed entirely from Visual Studio and VS Code without requiring a non-automated extra setup step (downloading/installing extra software)?

We got a few days to figure this out.

@misha130
Copy link
Contributor

misha130 commented Feb 1, 2020

I'll be willing to merge this provided no one comes up with an alternative that doesn't requi

I was looking at https://github.com/natemcmaster/dotnet-tools because of #51 and saw you have this: https://github.com/rendlelabs/dotnet-unpkg
And also this: https://docs.microsoft.com/en-us/aspnet/core/client-side/libman/libman-vs?view=aspnetcore-3.1#additional-resources

BUT you will still need to get npm so you could transpile JS for old browser support.

@raphaelschmitz00
Copy link

I'll be willing to merge this provided no one comes up with an alternative that doesn't require me to install node and npm, at least not manually and globally (at the machine level).

Well, the proposal under discussion is to use manually/globally installed npm in order to install npm packages as part of the C# build process.
What jumps out to me as alternatives immediately would be if you invert one of those parts in bold.

Alternative to use manually/globally installed npm
I have no experience with the npm NuGet package, but that seems like that would be exactly what this does.

Alternative to install npm packages as part of the C# build process
Use already built frontend files in the website project. Right now I can think of 2 options;
NuGet packages or git submodules.
This alternative has the advantage of keeping faster build times, but needs extra effort to make it practical to work for the frontend team.

@asynts
Copy link
Contributor Author

asynts commented Feb 2, 2020

Alternative to use manually/globally installed npm I have no experience with the npm NuGet package, but that seems like that would be exactly what this does.

That package was last updated in 2015 and wasn't official as far as I know. That link is like a landmine because it's the first google result for "install npm with nuget", @ranolfi suggested this too.

We could create our own nuget package for example Codidact.Npm and maintain this on our own. This would simplify the build process but adds that complexity elsewhere. I don't know if this would be a good idea.

Alternative to install npm packages as part of the C# build process Use already built frontend files in the website project. Right now I can think of 2 options; NuGet packages or git submodules.

I did some experimentation with nuget packages and the tool doesn't seem to be meant for it. I could not find a way to create a package that only contains a few files, there always has to be something build, this could be a dummy console application for example.

Submodules are promising, this was my original suggestion, but @misha130 suggested to use npm since we'll have to install babel at some point which would require npm anyways.

I don't like the npm stuff either, but really don't see another option.

@raphaelschmitz00

@DoctaJonez
Copy link
Member

DoctaJonez commented Feb 2, 2020

That package was last updated in 2015

That's a bit of a problem, there are other nuget feeds, such as the chocolatey one, but it's in the same state.

The Yarn package is being kept up to date. For those unfamiliar with Yarn, it's a fully compatible direct replacement for npm. The Facebook team developed it because they found the build times to be too high. It features parellelised downloads (which helps with the build times), and also gets around dependency version conflicts.

I don't know if yarn was already discussed, but it could be worth using it if it solves this problem for us. We don't want to get into the business of maintaining 3rd party packages.

Anyway, it's already bundled in a maintained nuget package on the chocolatey nuget repo (https://chocolatey.org/api/v2/). You can point any nuget client at that repo, it's a standard nuget repo. Installation via chocolatey is one of their primary recommended ways of installing yarn, so it stands to reason that the package will be kept up to date.

I did some experimentation with nuget packages and the tool doesn't seem to be meant for it. I could not find a way to create a package that only contains a few files, there always has to be something build, this could be a dummy console application for example.

Chocolatey does this, it's a nuget client for installing applications from nuget package feeds. I've used it successfully with our CI server for many years.

@asynts
Copy link
Contributor Author

asynts commented Feb 2, 2020

@DoctaJonez That looks very promising, however, it only seems to work in Windows. The website doesn't offer install instructions for Linux and the github repo says:

Other Platforms

Prerequisites:

  • Install and configure Mono 3.12.0 (3.8.0 should also work).
    [...]

(source)

Which is a bit weird, because the current stable version of Mono is 6.8.0.

@DoctaJonez
Copy link
Member

We'd have to do some investigation then, to see if it works with the latest release of mono. Would be better if it worked with .Net core.

@asynts
Copy link
Contributor Author

asynts commented Feb 2, 2020

We'd have to do some investigation then, to see if it works with the latest release of mono. Would be better if it worked with .Net core.

That seems to be a lost battle to me. It would only be viable if the installation would be simpler than yarn or npm. This does not seem to be the case.

@DoctaJonez
Copy link
Member

That seems to be a lost battle to me. It would only be viable if the installation would be simpler than yarn or npm. This does not seem to be the case.

Chocolatey was kind of an aside. My point was there is a Yarn package that we can use that is actively maintained in the Choco nuget feed. We can use the vanilla nuget client to install this.

@asynts
Copy link
Contributor Author

asynts commented Feb 3, 2020

I couldn't get this to work. Here is what I tried:

$ cd `mktemp -d`
$ dotnet new console
The template "Console Application" was created successfully.

Processing post-creation actions...
Running 'dotnet restore' on /tmp/tmp.iOMJgFc3FL/tmp.iOMJgFc3FL.csproj...
  Restore completed in 147.09 ms for /tmp/tmp.iOMJgFc3FL/tmp.iOMJgFc3FL.csproj.

Restore succeeded.

$ dotnet add package --source https://chocolatey.org/api/v2/ --version 1.21.1 yarn
  Writing /tmp/tmpFq2cpJ.tmp
info : Adding PackageReference for package 'yarn' into project '/tmp/tmp.iOMJgFc3FL/tmp.iOMJgFc3FL.csproj'.
info : Restoring packages for /tmp/tmp.iOMJgFc3FL/tmp.iOMJgFc3FL.csproj...
info : Package 'yarn' is compatible with all the specified frameworks in project '/tmp/tmp.iOMJgFc3FL/tmp.iOMJgFc3FL.csproj'.
info : PackageReference for package 'yarn' version '1.21.1' added to file '/tmp/tmp.iOMJgFc3FL/tmp.iOMJgFc3FL.csproj'.
info : Committing restore...
info : Generating MSBuild file /tmp/tmp.iOMJgFc3FL/obj/tmp.iOMJgFc3FL.csproj.nuget.g.props.
info : Writing assets file to disk. Path: /tmp/tmp.iOMJgFc3FL/obj/project.assets.json
log  : Restore completed in 211.88 ms for /tmp/tmp.iOMJgFc3FL/tmp.iOMJgFc3FL.csproj.
$ tree ~/.nuget/packages/yarn/1.21.1/
/home/me/.nuget/packages/yarn/1.21.1/
├── tools
│   └── chocolateyinstall.ps1
├── yarn.1.21.1.nupkg
├── yarn.1.21.1.nupkg.sha512
└── yarn.nuspec

1 directory, 4 files
$ cat ~/.nuget/packages/yarn/1.21.1/tools/chocolateyinstall.ps1
$ErrorActionPreference = 'Stop'; # stop on all errors

$packageName = 'yarn'

$packageArgs = @{
  packageName = $packageName
  softwareName = 'Yarn*'
  fileType = 'msi'
  silentArgs = "/qn /norestart"
  validExitCodes = @(0, 3010, 1641)
  checksumType = 'sha256'

  url = 'https://yarnpkg.com/downloads/1.21.1/yarn-1.21.1.msi'
  checksum = '95EB5EB485740788FE7BAE57AF403F71395139802A228A88D1AFE290891A1344'
}

Install-ChocolateyPackage @packageArgs

# Update Yarn's package.json file so it can tell that it was installed via Chocolatey.
if (Test-Path "${env:ProgramFiles(x86)}\Yarn\package.json") {
  $path = "${env:ProgramFiles(x86)}\Yarn\package.json"
} else {
  $path = "$env:ProgramFiles\Yarn\package.json"
}

$script = @"
  (Get-Content -Path '$path') ``
    -replace 'installationMethod":.+', 'installationMethod": "choco",' ``
    | Set-Content '$path'
"@

Start-ChocolateyProcessAsAdmin -Statements $script

if (-Not (Get-Command "node" -errorAction SilentlyContinue)) {
  Write-Host "Yarn requires NodeJS to be installed. To install, use either of the commands below:"
  Write-Host "choco install nodejs"
  Write-Host "choco install nodejs-lts"
}

It doesn't seem to install anything useful, really.

@misha130
Copy link
Contributor

misha130 commented Feb 3, 2020

...and saw you have this: https://github.com/rendlelabs/dotnet-unpkg

My comment here was ignored and unpgk is a viable option. @luap42 already put co-design on unpkg. Why not use it?

@asynts
Copy link
Contributor Author

asynts commented Feb 4, 2020

@misha130 That tool was last updated in 2018. It also relies on .NET Core 2.1 which would have to be installed first. So instead of adding npm to the install instructions we'd add .NET Core 2.1 to the install instructions.

However, the unpkg CDN might help us, I just noticed that msbuild has a DownloadFile task, thus we could do something like:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
    <!-- ... -->

    <Target Name="DownloadCodesign" BeforeTargets="Build">
        <DownloadFile
            SourceUrl="https://unpkg.com/@codidact/co-design@0.4.0/dist/codidact.css"
            DestinationFolder="wwwroot/css" />
    </Target>
</Project>

The problem is that this won't work offline at all. For npm you need a network connection at least once and it's even possible to avoid that if you install the packages manually.

@ranolfi
Copy link
Member

ranolfi commented Feb 5, 2020

Thank you everyone for your valuable insights - not just here but also on Discord chat.

Pulling from the unpkg CDN with LibMan, in particular, seems to be a sound strategy in a lot of scenarios (read: in the near future when we have additional dependencies for other tasks).

Still, for codebases we have complete control over, such as co-design here, I'm proposing we stick to the pure-git strategy.

It boils down to:

  • release co-design
  • publish to npm
  • pull from npm in 'core'

vs

  • release co-design
  • update .gitmodules to reference the newer release tag

Contributors have git already, so this appears to make more sense.

I can submit a PR later, unless @asynts or someone else wishes to step in instead.

@asynts asynts mentioned this pull request Feb 11, 2020
@asynts
Copy link
Contributor Author

asynts commented Mar 15, 2020

To avoid misunderstandings: this pull request should not be merged, at least not as it is now.

I am planning to update this pull request tomorrow using submodules.

Copy link
Member

@ArtOfCode- ArtOfCode- left a comment

Choose a reason for hiding this comment

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

Blocking merge on asynts' suggestion. Poke me in Discord when this is good to go.

@asynts
Copy link
Contributor Author

asynts commented Mar 17, 2020

I am closing this because we still have the issue open and when the artifacts are ready in co-design, I would create a new pull request anyways, this one is a bit NPM-ish.

@asynts asynts closed this Mar 17, 2020
@asynts asynts deleted the asynts/45/codesign-with-npm branch March 29, 2020 10:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use submodule for Codesign, don't include the sources.
7 participants