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

Improved development environment #101

Closed
wants to merge 5 commits into from

Conversation

solidsloth
Copy link

What does this PR do?

In development the application can now be run to proxy to the react development server. This resolves the problem where the code needs to be changed in development to point to the server correcly (which is running on a different port). I also modified the csproj file to have msbuild copy the required front-end files to the publish folder when needed. Therefore the built files no longer need to be copied over and permanently stored in the wwwroot because in development the requests are proxied, and in production the files are copied to the publish folder.

Closes Issue(s)

I think EnsureCreated this might help with issue #96 because the database is now created in development.

Motivation

My main motiviation was to simplify the development and resole the issue about bend URLs being broken in develop mode. This allows the same workflow: yarn develop + dotnet run without needing to change any urls in components/SearchResults.tsx or components/DisplayPackage.tsx

Additional Notes

  • I fixed the vscode launch.json file to work with the new netcoreapp2.1.
  • I added a few artifacts to the gitignore to prevent accidental commits of the generated files, including: the .cache folder, the dist folder, and the SQLite database file.
  • I removed the generated source files from the repo since these should be rebuilt upon every release.

In development the application can now be run to proxy to the react
development server. This resolves the problem where the code needs to be
changed in development to point to the server correcly (which is running
on a different port). I also modified the csproj file to have msbuild
copy the required front-end files to the publish folder when needed.
Therefore the built files no longer need to be copied over and
permanently stored in the wwwroot because in development the requests
are proxied, and in production the files are copied to the publish
folder.
@loic-sharma
Copy link
Owner

Fantastic pull request! I'll play around with this tonight and make sure that everything is in tip-top shape. Thank you @solidsloth!

@solidsloth
Copy link
Author

I have an email about you commenting on the context.Database.EnsureCreated(); call, but can't seem to find that comment here.

Good call, I was not aware of the issue with migrations. I added the like because I was not able to run the app from dotnet publish because the database was never created. I think I was running with Release configuration, which may have been what caused the problem. I'll have to see if there's a better way to handle this. Could we just move the existing DbContext.Database.Migrate() outside of the if (env.IsDevelopment()) block to ensure it's created regardless of the build configuration?

@loic-sharma
Copy link
Owner

loic-sharma commented Sep 28, 2018

I have an email about you commenting on the context.Database.EnsureCreated(); call, but can't seem to find that comment here.

Sorry about that, I deleted my comment as I wanted to give this problem some more thought.

Could we just move the existing DbContext.Database.Migrate() outside of the if (env.IsDevelopment()) block to ensure it's created regardless of the build configuration?

I would rather allow users to opt-out of this. What if I deploy two instances of BaGet and both try to run migrations? Maybe a better approach be to add a configuration that runs/disables migrations at start-up, regardless of the build configuration?

@solidsloth
Copy link
Author

I was thinking the same thing after I commented. Might be good to add some configuration flag that would enable or disable the migrate.

Might be best I just revert this portion of the code since it seems irrelevant to this particular PR and I can make a separate request or issue that we can figure something out on.

@tomzo
Copy link
Contributor

tomzo commented Sep 30, 2018

@loic-sharma would be nice if you merged this soon, so that I can rebase #103 on top of it and setup running npm in pipeline.

tomzo added a commit to ai-traders/BaGet that referenced this pull request Oct 1, 2018
tomzo added a commit to ai-traders/BaGet that referenced this pull request Oct 1, 2018
@tomzo tomzo mentioned this pull request Oct 1, 2018
@loic-sharma
Copy link
Owner

I tested this on my macbook and ran into some issues. Building a docker image results in the following error:

/src/src/BaGet/BaGet.csproj(43,5): error MSB6003: The specified task executable "sh" could not be run. The working directory "../Baget.UI/" does not exist.
The command '/bin/sh -c dotnet publish src/BaGet -c ${APP_BUILD} -o /app' returned a non-zero code: 1

It looks like this is because the SpaRoot property in BaGet.csproj is ..\BaGet.UI. We may need an msbuild variable here to make the path explicit.

Also, when I run dotnet publish from a clean git clone I get:

Microsoft (R) Build Engine version 15.7.179.6572 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restore completed in 52.62 ms for /Users/loshar/Code/sloth-baget/src/BaGet.Core/BaGet.Core.csproj.
  Restore completed in 52.62 ms for /Users/loshar/Code/sloth-baget/src/BaGet.Azure/BaGet.Azure.csproj.
  Restore completed in 56.84 ms for /Users/loshar/Code/sloth-baget/src/BaGet.Web/BaGet.Web.csproj.
  Restore completed in 56.83 ms for /Users/loshar/Code/sloth-baget/src/BaGet/BaGet.csproj.
  BaGet.Core -> /Users/loshar/Code/sloth-baget/src/BaGet.Core/bin/Debug/netstandard2.0/BaGet.Core.dll
  BaGet.Azure -> /Users/loshar/Code/sloth-baget/src/BaGet.Azure/bin/Debug/netstandard2.0/BaGet.Azure.dll
  BaGet.Web -> /Users/loshar/Code/sloth-baget/src/BaGet.Web/bin/Debug/netcoreapp2.1/BaGet.Web.dll
  BaGet -> /Users/loshar/Code/sloth-baget/src/BaGet/bin/Debug/netcoreapp2.1/BaGet.dll
  up to date in 3.571s
  
  > baget@1.0.0 build /Users/loshar/Code/sloth-baget/src/BaGet.UI
  > parcel build ./index.html
  
  🚨  /Users/loshar/Code/sloth-baget/src/BaGet.UI/index.html: Cannot destructure property `parser` of 'undefined' or 'null'.
      at new MakeTranspileAsset (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/parcel-plugin-typescript/build/frontend/assets/typescript.js:7:24)
      at Parser.getAsset (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/parcel-bundler/src/Parser.js:85:12)
      at HTMLAsset.addURLDependency (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/parcel-bundler/src/Asset.js:116:8)
      at HTMLAsset.processSingleDependency (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/parcel-bundler/src/assets/HTMLAsset.js:99:26)
      at ast.walk.node (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/parcel-bundler/src/assets/HTMLAsset.js:166:43)
      at traverse (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/posthtml/lib/api.js:105:26)
      at traverse (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/posthtml/lib/api.js:111:5)
      at traverse (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/posthtml/lib/api.js:105:17)
      at traverse (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/posthtml/lib/api.js:111:5)
      at traverse (/Users/loshar/Code/sloth-baget/src/BaGet.UI/node_modules/posthtml/lib/api.js:105:17)
  npm ERR! code ELIFECYCLE
  npm ERR! errno 1
  npm ERR! baget@1.0.0 build: `parcel build ./index.html`
  npm ERR! Exit status 1
  npm ERR! 
  npm ERR! Failed at the baget@1.0.0 build script.
  npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
  
  npm ERR! A complete log of this run can be found in:
  npm ERR!     /Users/loshar/.npm/_logs/2018-10-02T06_48_54_335Z-debug.log
/Users/loshar/Code/sloth-baget/src/BaGet/BaGet.csproj(49,5): error MSB3073: The command "npm run build" exited with code 1.

How did you get around these issues?

@loic-sharma
Copy link
Owner

@tomzo Agreed. I'd like to get this merged in ASAP as well :)

@solidsloth
Copy link
Author

solidsloth commented Oct 2, 2018

@loic-sharma I don't think I'll have time to look at this tonight. This error looks familiar. I want to say this is actually why I was using yarn install in the csproj file. Can you try switching that back and see if it fixes it?

Not sure about the docker issue. I was worried the .. path would cause issues in places. I couldn't find a good way to get a better path in msbuild. It would be easier to just move the BaGet.UI folder underneath the BaGet folder, but that would obviously be a pretty massive change to the project structure. Admittedly I haven't looked too much into how the docker image is being built, so I'm not sure if something can be changed there to resolve the issue.

@solidsloth
Copy link
Author

Also, I think changing WORKDIR /src to WORKDIR /src/src/BaGet and changing the dotnet commands to run in the cwd (like a plain dotnet restore) should resolve the docker build.

@tomzo
Copy link
Contributor

tomzo commented Oct 3, 2018

I have changed pretty much everything about how docker is built in #105 so I don't think it makes sense to make fixes so that this primitive Dockerfile build works here.
I am sure I can integrate this further in the pipeline once it is merged.
I thought this was supposed to be useful in Debug configuration to quickly rebuild SPA, not for building final image.

@solidsloth
Copy link
Author

@tomzo I was wondering about this since I knew you were working on something, thanks.

@tomzo
Copy link
Contributor

tomzo commented Oct 4, 2018

@solidsloth I have integrated your changes (and intended outcome) into #108, let me know if you are fine with it and can close this.

@solidsloth
Copy link
Author

@tomzo that's great, thanks. This way we don't have to break docker in the mean time.

@solidsloth solidsloth closed this Oct 4, 2018
loic-sharma added a commit that referenced this pull request Nov 11, 2018
Rebuilds the frontend on the publish step. This is based off @solidsloth's work in #101.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants