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
build(aio): big move of docs related files #14361
Conversation
|
|
I was thinking that aio should have its own |
@wardbell - that is mostly where this PR puts us. But I didn't want to do too much refactoring in such a monster of a commit. |
@@ -3,7 +3,6 @@ | |||
/dist/ | |||
node_modules | |||
bower_components | |||
angular.io/dist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it is covered further down by the /aio/webapp/src/content/docs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that @petebacondarwin is right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see it. I agree it existed twice before, but you seem to have removed both occurences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I just realised (I think) that the ng build
process will compile to the dist folder; and that is where you are deploying to firebase from... I'll put that gitignore back in.
In the meantime it also means that I am not sure that ng build
will be copying the generated docs to that folder....
scripts/ci-lite/test_aio.sh
Outdated
# Lint the code | ||
cd "`dirname $0`/../../angular.io" | ||
cd "`dirname $0`/../../aio" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this go to /aio/webapp
(here and elsewhere)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so because the yarn is running off the package.json
, which I moved to the /aio
folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think I need to change the package.json
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, there are files (e.g. aio/webapp/src/tsconfig.json
, aio/webapp/e2e/tsconfig.json
, aio/webapp/src/angular-cli.json
) which assume (afaict) that the project root (i.e. the node_modules/
directory, package.json etc) will be aio/webapp/
. If we are moving the project root to aio/
, when these files (and probably others) need to be updated accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found out that it is not possible to specify the working directory (cwd) for the firebase command line tool. It must be run from the correct folder :-(
So the options are:
a) move the package.json back inside the webapp folder, next to the firebase.json file; which would mean that the stuff outside that folder does not benefit from the yarn install.
b) move the firebase.json out of the webapp folder; I think this might be the best option
c) change the package.json scripts to cd into that folder before running the firebase tool; I think this is not nice and probably would break on Windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once we resolve the small things I pointed out.
aio/gulpfile.js
Outdated
// THIS CHECK SHOULD BE THE FIRST THING IN THIS FILE | ||
// This is to ensure that we catch env issues before we error while requiring other dependencies. | ||
require('../tools/check-environment')({ | ||
requiredNpmVersion: '>=3.10.7 <4.0.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't use npm in aio
aio/gulpfile.js
Outdated
// This is to ensure that we catch env issues before we error while requiring other dependencies. | ||
require('../tools/check-environment')({ | ||
requiredNpmVersion: '>=3.10.7 <4.0.0', | ||
requiredNodeVersion: '>=6.9.5 <7.0.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we didn't duplicate this in /aio because now we have a yet another place to update the node version in this repo. could we reuse the top level check?
aio/jsconfig.json
Outdated
@@ -0,0 +1,7 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the purpose of this file? IDE support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we document it somehow? e.g. using "comment": "the purpose of this file is... "
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
98a8033
to
791bf39
Compare
All the docs related files (docs-app, doc-gen, content, etc) are now to be found inside the `/aio` folder. The related gulp tasks have been moved from the top level gulp file to a new one inside the `/aio` folder. The structure of the `/aio` folder now looks like: ``` /aio/ build/ # gulp tasks content/ #MARKDOWN FILES for devguides, cheatsheet, etc devguides/ cheatsheets/ transforms/ #dgeni packages, templates, etc src/ app/ assets/ content/ #HTML + JSON build artifacts produced by dgeni from /aio/content. #This dir is .gitignored-ed e2e/ #protractor tests for the doc viewer app node_modules/ #dependencies for both the doc viewer builds and the dgeni stuff #This dir is .gitignored-ed gulpfile.js #Tasks for generating docs and building & deploying the doc viewer ``` Closes angular#14361
791bf39
to
cb6d2b6
Compare
CLAs look good, thanks! |
All the docs related files (docs-app, doc-gen, content, etc) are now to be found inside the `/aio` folder. The related gulp tasks have been moved from the top level gulp file to a new one inside the `/aio` folder. The structure of the `/aio` folder now looks like: ``` /aio/ build/ # gulp tasks content/ #MARKDOWN FILES for devguides, cheatsheet, etc devguides/ cheatsheets/ transforms/ #dgeni packages, templates, etc src/ app/ assets/ content/ #HTML + JSON build artifacts produced by dgeni from /aio/content. #This dir is .gitignored-ed e2e/ #protractor tests for the doc viewer app node_modules/ #dependencies for both the doc viewer builds and the dgeni stuff #This dir is .gitignored-ed gulpfile.js #Tasks for generating docs and building & deploying the doc viewer ``` Closes angular#14361
All the docs related files (docs-app, doc-gen, content, etc) are now to be found inside the `/aio` folder. The related gulp tasks have been moved from the top level gulp file to a new one inside the `/aio` folder. The structure of the `/aio` folder now looks like: ``` /aio/ build/ # gulp tasks content/ #MARKDOWN FILES for devguides, cheatsheet, etc devguides/ cheatsheets/ transforms/ #dgeni packages, templates, etc src/ app/ assets/ content/ #HTML + JSON build artifacts produced by dgeni from /aio/content. #This dir is .gitignored-ed e2e/ #protractor tests for the doc viewer app node_modules/ #dependencies for both the doc viewer builds and the dgeni stuff #This dir is .gitignored-ed gulpfile.js #Tasks for generating docs and building & deploying the doc viewer ``` Closes angular#14361
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
All the docs related files (docs-app, doc-gen, content, etc)
are now to be found inside the
/aio
folder.The related gulp tasks have been moved from the top level
gulp file to a new one inside the
/aio
folder.The structure of the
/aio
folder now looks like: