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

[Doc] buildpack ref docs #2508

Merged
merged 26 commits into from Aug 4, 2020
Merged

[Doc] buildpack ref docs #2508

merged 26 commits into from Aug 4, 2020

Conversation

jcalcaben
Copy link
Contributor

@jcalcaben jcalcaben commented Jun 24, 2020

Description

This PR updates the JSDocs in the targets in the Buildpack package.
It also creates a file in devdocs that pulls this information into a reference-style topic.

Related Issue

Closes PWA-703

Acceptance

@zetlen

Verification Stakeholders

@zetlen

Specification

Verification Steps

  1. Navigate to the pwa devdocs directory: cd pwa-devdocs

  2. Start the HTML preview server: yarn develop

  3. Verify files are auto-generated:

    • src/_includes/auto-generated/pwa-buildpack/lib/WebpackTools/ModuleTransformConfig.md
    • src/_includes/auto-generated/pwa-buildpack/lib/BuildBus/declare-base.md
    • src/_includes/auto-generated/pwa-buildpack/lib/BuildBus/BuildBus.md
    • src/_includes/auto-generated/pwa-buildpack/lib/BuildBus/TargetProvider.md
    • src/_includes/auto-generated/pwa-buildpack/lib/BuildBus/Target.md
    • src/_includes/auto-generated/pwa-buildpack/lib/Utilities/getEnvVarDefinitions.js
  4. On the devdocs preview site, in the top navigation, click on PWA Buildpack under the Reference API item.

  5. Verify new navigation layout

  6. Verify all the links under the Extension points, Extension framework, Developer utilities, Webpack tools, and Environment configuration sections work.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have updated the documentation accordingly, if necessary.

@jcalcaben jcalcaben added pkg:pwa-devdocs documentation This pertains to documentation. version: Minor This changeset includes functionality added in a backwards compatible manner. docs documentation labels Jun 24, 2020
@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Jun 24, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jun 24, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-703.

Generated by 🚫 dangerJS against 89a63cd

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented Jun 24, 2020

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2508.pwa-venia.com : LH Performance Expected 0.85 Actual 0.34, LH Accessibility Expected 1 Actual 0.97, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 90 Actual 39.333333333333
https://pr-2508.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.34, LH Best Practices Expected 1 Actual 0.92
https://pr-2508.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.39, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 65 Actual 50

Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

Partial review for @jcalcaben because I'm taking so long with this process. There's enough here to start a conversation--I'll continue to review this and the other two after submitting.

* for dependencies with Targets to interact with each other.
*
* Manages dependency participation in project builds and tasks.
* It connects dependencies with Targets and lets them interact with each other.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to add something like executes their declare and intercept files so they can interact with each other.

packages/pwa-buildpack/lib/BuildBus/Target.js Outdated Show resolved Hide resolved
packages/pwa-buildpack/lib/BuildBus/Target.js Outdated Show resolved Hide resolved
packages/pwa-buildpack/lib/BuildBus/Target.js Outdated Show resolved Hide resolved
packages/pwa-buildpack/lib/BuildBus/Target.js Show resolved Hide resolved
* Respond to a request from a [TargetProvider]{@link https://pwastudio.io/pwa-buildpack/reference/buildbus/targetprovider/}
* to retrieve a different(external) TargetProvider.
*
* When using a TargetProvider disconnected from a
Copy link
Contributor

Choose a reason for hiding this comment

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

I sort of regret this aside, because I can't think of a single reason you'd use a TargetProvider disconnected from a BuildBus. Maybe this should just say "This callback pattern helps to loosely couple TargetProviders so they are more testable."

packages/pwa-buildpack/lib/BuildBus/TargetProvider.js Outdated Show resolved Hide resolved
Comment on lines 52 to 56
* Collects requests to intercept and modify individual files from this
* dependency. Only files from the currently requesting dependency may
* dependency.
*
* Only files from the currently requesting dependency may
* be transformed.
Copy link
Contributor

Choose a reason for hiding this comment

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

What I wrote is no longer accurate. We added an exception for interceptor files in the project itself (e.g. venia-concept) to be able to transform ANY file from ANY dependency. It's only the dependencies (e.g. venia-ui) that are prevented from modifying files from other dependencies (e.g. peregrine).

We did this because a custom interceptor file for the project should be able to do whatever it wants, since it's presumably maintained by the project author who is also in charge of importing dependencies.

Can you help me to put that here?

@@ -196,3 +179,65 @@ module.exports = targets => {

targets.declare(builtins);
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh, this makes sense. Thanks for this reorg.

@m2-community-project m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Jul 23, 2020
Co-authored-by: James Zetlen <jzetlen@adobe.com>
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

Finishing up the first round. Thanks for your patience.

packages/pwa-buildpack/lib/BuildBus/declare-base.js Outdated Show resolved Hide resolved
packages/pwa-buildpack/lib/BuildBus/declare-base.js Outdated Show resolved Hide resolved
* Since the storefront developer is in charge of important dependencies,
* the interceptor files in the storefront project itself should be able to
* transform ANY file from ANY dependency.
* However, interceptor files in the storefront dependencies are prevented
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +2 to +5
{
target: 'pwa-buildpack/lib/BuildBus/declare-base.js',
type: 'function'
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this belong here? The declare-base.js file is what declares Buildpack targets, so we wrote the doc blocks in that file. But the targets themselves are in a different category of thing, which should be documented in a separate "Targets" section, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't add another level of nesting to the navigation, so they will all need to go under "Extensibility"

pwa-devdocs/src/_data/buildpack-api.yml Outdated Show resolved Hide resolved
Comment on lines 3 to 7
- label: BuildBus
url: /pwa-buildpack/reference/buildbus/
entries:
- label: Extensibility Targets
url: /pwa-buildpack/reference/buildbus/targets/
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that I called the directory BuildBus in source code, but I think in documentation, the top-level item should be called "Extensibility" and there should be an additional entry below, called "BuildBus".

Comment on lines 39 to 41
- label: ModuleTransformConfig
url: /pwa-buildpack/reference/moduletransformconfig/

Copy link
Contributor

Choose a reason for hiding this comment

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

This belongs under Extensibility

Co-authored-by: James Zetlen <jzetlen@adobe.com>
James Zetlen and others added 3 commits July 24, 2020 12:58
* docs: elaborate on envVarDefinitions API

* Apply suggestions from code review

Co-authored-by: James Zetlen <jzetlen@adobe.com>

* Update pwa-devdocs/src/pwa-buildpack/reference/environment-variables/index.md

Co-authored-by: James Calcaben <jcalcaben@users.noreply.github.com>

* Update pwa-devdocs/src/pwa-buildpack/reference/environment-variables/index.md

Co-authored-by: James Calcaben <jcalcaben@users.noreply.github.com>

* docs: elaborate on envVarDefinitions API

Co-authored-by: James Calcaben <jcalcaben@users.noreply.github.com>
James Zetlen and others added 2 commits July 27, 2020 11:08
zetlen
zetlen previously approved these changes Jul 27, 2020
packages/pwa-buildpack/lib/BuildBus/Target.js Outdated Show resolved Hide resolved
@jcalcaben jcalcaben requested a review from zetlen August 3, 2020 19:55
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@m2-community-project m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Aug 4, 2020
@dpatil-magento
Copy link
Contributor

Verification steps look good.

@dpatil-magento dpatil-magento merged commit 8fc82a3 into develop Aug 4, 2020
@dpatil-magento dpatil-magento deleted the jimothy/buildpack-ref-docs branch August 4, 2020 16:59
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs documentation documentation This pertains to documentation. pkg:pwa-buildpack pkg:pwa-devdocs version: Minor This changeset includes functionality added in a backwards compatible manner.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants