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

Devdocs: serve the content of README.md files from the search index module #38784

Merged
merged 3 commits into from
Jan 13, 2020

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Jan 10, 2020

While working on #38190 and updating the devdocs/bin/generate-devdocs-index script, I discovered that in addition to the search index, we also store the text content of the indexed files in the generated search-index.js module. It's used to generate snippets (that contain the searched term) for the search results. This text content is plain text with all markup stripped.

If we stored the raw Markdown content to this module, too, we could use it to serve the README.md files content in the /devdocs/service/content endpoint. This PR implements exactly that. Currently it's served from the Calypso source tree that needs to be present at runtime.

I think this is a better and more elegant solution than moving the Markdown content to the client webpack bundle that @ockham proposes in #35833. Mainly because it doesn't inflate the webpack manifest with 800+ new chunks.

It also makes the Calypso server more self-contained: soon we'll need only the build/ and public/ directory to run the Calypso server, and not the whole source tree.

Changes proposed in this Pull Request

  1. Move the generating script to the /bin directory and give it a .js suffix. That's a better location and the .js extension will make it Prettier-ed and ESLint-ed.
  2. In package.json, make sure that the build-devdocs:* tasks are finished before build-server starts. build-server uses the files generated by build-devdocs:*. Until now, they were running in parallel and there was a race condition present.
  3. In package.json, simplify and rename the build-devdocs:index task. The script doesn't need any special environment variables and can be run with a simple node command.
  4. In the generate-devdocs-index.js script, format it with Prettier and fix ESLint errors. And also store the raw content as a content property of the generated document records.
  5. Finally, in the implemenation of the /devdocs/service/endpoint, use searchIndex.documents[ idx ].content to serve the raw content. And remove the FS access.

Testing instructions

  • test that the README search at /devdocs works correctly
  • test that the <ReadmeViewer> component that's used across devdocs shows the README documents as expected.

@jsnajdr jsnajdr requested a review from a team as a code owner January 10, 2020 21:54
@matticbot
Copy link
Contributor

@jsnajdr jsnajdr requested review from sgomes, ockham and sirreal January 10, 2020 21:55
@jsnajdr jsnajdr self-assigned this Jan 10, 2020
@jsnajdr jsnajdr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build DevDocs labels Jan 10, 2020
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

server/devdocs/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Nice! This has a much smaller footprint than mine!

Unfortunately, none of the docs in devdocs/docs/ are currently working (The Calypso Guide, Accessibility, Color, Icons); same for Contributing (.github/CONTRIBUTING.md -- maybe see #36167?)

(Typography, which is in devdocs/, does work, as doe component and blocks docs -- except for the ones that we've moved into packages/components, but that's a known and pre-existing issue.)

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 13, 2020

Unfortunately, none of the docs in devdocs/docs/ are currently working

Oh, I misunderstood what path.relative() really does and didn't test the patch sufficiently 🙁 Fixed in f5aac84

except for the ones that we've moved into packages/components

Files in packages/ were not part of the search index until now -- that's fixed by this added line: https://github.com/Automattic/wp-calypso/pull/38784/files#diff-8434269b829266816be9c69918de4979R24

Displaying their content now works, too -- I don't see any regressions or bugs for packages/components/

All issues from the review should be addressed now.

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Thanks @jsnajdr, this is working great now!

@jsnajdr jsnajdr merged commit 07afee8 into master Jan 13, 2020
@jsnajdr jsnajdr deleted the devdocs/serve-content-from-search-index branch January 13, 2020 13:35
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants