Skip to content

[RFC] refactor(docs): move apidocs to /docs #1207

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.

Sign up for GitHub

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

Jump to bottom
Merged
raymondfeng merged 3 commits into from
Apr 11, 2018
Merged

[RFC] refactor(docs): move apidocs to /docs #1207

raymondfeng merged 3 commits into from
Apr 11, 2018

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Mar 28, 2018 •
edited
Loading

This PR changes how api docs are generated and published.

  1. Generate api docs for each package into docs/api-docs/<pkg>
  • Each package now has `docs/api-docs/.html
  • No more duplicate assets for fonts/css/js
  • @loopback/docs itself servers as index.html for all packages
  1. Publish api docs as part of @loopback/docs
  2. Remove api-docs from each published packages to reduce bundle sizes (node_modules after production build is too large #1134)

We'll adjust apidocs.loopback.io site to use @loopback/docs for LoopBack 4 api docs.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

@raymondfeng raymondfeng force-pushed the move-apidocs-to-docs branch from 2f1bab8 to 6dd574f Compare March 28, 2018 23:40
@raymondfeng raymondfeng changed the title Move apidocs to docs [RFC] Move apidocs to docs Mar 28, 2018
@raymondfeng raymondfeng force-pushed the move-apidocs-to-docs branch 2 times, most recently from fb40a51 to 18d281a Compare March 29, 2018 03:18
b-admike
b-admike reviewed Mar 29, 2018
View reviewed changes
packages/build/bin/generate-apidocs.js
if (!utils.isOptionSet(apidocsOpts, '--out', '-o')) {
args.push('-o', 'api-docs');
let out = 'api-docs';
if (process.env.LERNA_ROOT_PATH) {
Copy link
Contributor

@b-admike b-admike Mar 29, 2018

Choose a reason for hiding this comment

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

Is this dependent on #1189 landing?

Copy link
Contributor Author

@raymondfeng raymondfeng Mar 29, 2018

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

@raymondfeng raymondfeng Mar 30, 2018

Choose a reason for hiding this comment

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

#1189 is landed. I rebased the PR to master.

bajtos
bajtos reviewed Mar 29, 2018
View reviewed changes
packages/build/bin/generate-apidocs.js
if (!utils.isOptionSet(apidocsOpts, '--out', '-o')) {
args.push('-o', 'api-docs');
let out = 'api-docs';
if (process.env.LERNA_ROOT_PATH) {
Copy link
Member

@bajtos bajtos Mar 29, 2018

Choose a reason for hiding this comment

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

Could you please remind me: which code/script is setting LERNA_ROOT_PATH?

Copy link
Contributor Author

@raymondfeng raymondfeng Mar 29, 2018

Choose a reason for hiding this comment

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

See #1189

bajtos
bajtos reviewed Mar 29, 2018
View reviewed changes
packages/openapi-spec-builder/package.json Outdated
"build:apidocs": "lb-apidocs",
"clean": "lb-clean loopback-openapi-spec-builder*.tgz dist package api-docs",
"prepublishOnly": "npm run build && npm run build:apidocs",

Copy link
Member

@bajtos bajtos Mar 29, 2018

Choose a reason for hiding this comment

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

Could you please remove these empty lines? They will be eventually removed by npm anyways.

@bajtos
Copy link
Member

bajtos commented Mar 29, 2018

I am not able to fully consider ramifications of this change and whether it may break anything. If we agree this is the direction we want to take, then the code changes look mostly good to me - see my comments above.

@raymondfeng raymondfeng force-pushed the move-apidocs-to-docs branch from 18d281a to 2c7a030 Compare March 29, 2018 18:37
@raymondfeng raymondfeng force-pushed the move-apidocs-to-docs branch 2 times, most recently from de24cf2 to 6f83714 Compare March 29, 2018 21:16
@raymondfeng raymondfeng changed the base branch from add-lb-env to master March 30, 2018 00:03
@raymondfeng raymondfeng force-pushed the move-apidocs-to-docs branch 5 times, most recently from 97e662c to eaf5a3e Compare March 30, 2018 02:15
@raymondfeng raymondfeng changed the title [RFC] Move apidocs to docs [RFC] refactor(docs): move apidocs to /docs Mar 30, 2018
shimks
shimks reviewed Mar 30, 2018
View reviewed changes
packages/example-hello-world/package.json Outdated
"build:apidocs": "lb-apidocs",
"clean": "lb-clean *example-hello-world*.tgz dist package api-docs",
"prepublishOnly": "npm run build && npm run build:apidocs",

Copy link
Contributor

@shimks shimks Mar 30, 2018

Choose a reason for hiding this comment

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

nitpick: no new line

Copy link
Contributor Author

@raymondfeng raymondfeng Mar 30, 2018

Choose a reason for hiding this comment

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

fixed

packages/openapi-v3-types/package.json Outdated
"build:apidocs": "lb-apidocs",
"clean": "lb-clean loopback-openapi-v3-types*.tgz dist package api-docs",
"prepublishOnly": "npm run build && npm run build:apidocs",

Copy link
Contributor

@shimks shimks Mar 30, 2018

Choose a reason for hiding this comment

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

nitpick: no new line

Copy link
Contributor Author

@raymondfeng raymondfeng Mar 30, 2018

Choose a reason for hiding this comment

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

fixed

@raymondfeng raymondfeng force-pushed the move-apidocs-to-docs branch 4 times, most recently from 3bdfdc8 to 4e6c355 Compare March 30, 2018 22:22
@raymondfeng raymondfeng force-pushed the move-apidocs-to-docs branch from 4e6c355 to 9e415ca Compare April 9, 2018 15:26
bajtos
bajtos reviewed Apr 9, 2018
View reviewed changes
package.json Outdated
"pretest": "npm run clean && npm run build",
"test": "node packages/build/bin/run-nyc npm run mocha --scripts-prepend-node-path",
"mocha": "node packages/build/bin/run-mocha \"packages/*/DIST/test/**/*.js\" \"examples/*/DIST/test/**/*.js\" \"packages/cli/test/*.js\"",
"mocha": "node packages/build/bin/run-mocha \"packages/*/DIST/test/**/*.js\" \"examples/*/DIST/test/**/*.js\" \"packages/cli/test/*.js\" \"packages/build/test/integration/*.js\"",
Copy link
Member

@bajtos bajtos Apr 9, 2018

Choose a reason for hiding this comment

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

Would you mind making this more future-proof by using \"packages/build/test/*/*.js\"? I.e. allow any test category, not only integration. Unless I am missing something and the wildcard won't work.

Copy link
Contributor Author

@raymondfeng raymondfeng Apr 9, 2018

Choose a reason for hiding this comment

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

I wanted to exclude packages/build/test/fixtures. We can move fixtures to be inside integration.

@raymondfeng raymondfeng force-pushed the move-apidocs-to-docs branch from 9e415ca to 0190dd3 Compare April 9, 2018 16:11
@raymondfeng raymondfeng force-pushed the move-apidocs-to-docs branch from 0190dd3 to 1ae68fa Compare April 11, 2018 16:06
@raymondfeng raymondfeng merged commit 2a3f560 into master Apr 11, 2018
@dhmlau dhmlau mentioned this pull request Apr 30, 2018
Closed
23 tasks
@bajtos bajtos deleted the move-apidocs-to-docs branch June 5, 2018 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jannyHou jannyHou Awaiting requested review from jannyHou

@virkt25 virkt25 Awaiting requested review from virkt25

3 more reviewers

@bajtos bajtos bajtos left review comments

@b-admike b-admike b-admike left review comments

@shimks shimks shimks approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@raymondfeng @bajtos @b-admike @shimks