update find-comment string

[Entkenntnis]

#44 has changed the comment string to include the repo name.

This is currently breaking the find-comment action in the workflow.

[jzxhuang]

+1, this actually broke all existing usage of the action

An easy fix would be to update the template:

body-includes: '<!-- __NEXTJS_BUNDLE -->'

to

body-includes: '<!-- __NEXTJS_BUNDLE_<your-package.json-name> -->'

I just made this change on our team's GH action.

However, a better fix would probably be to make the monorepo support changes from #44 a manual opt-in?

cc @brkalow @dstaley

[dstaley]

Hi @Entkenntnis and @jzxhuang!

We're working on a new release with an updated workflow that no longer relies on the comment string which should be more reliable. In the meantime, you have two options (in order of preference):

1. Pin the version of nextjs-bundle-analysis used by running npm install --save-dev nextjs-bundle-analysis@0.4.0. This ensures that future breaking changes do not break your workflow.
2. Update all instances of npx -p nextjs-bundle-analysis in your workflow file to use an explicit version (npx -p nextjs-bundle-analysis@0.4.0).

If you've adjusted your body-includes as @jzxhuang mentioned, we still recommend that you pin the version of nextjs-bundle-analysis used to 0.5.0 since future versions will break your workflow. As a reminder v0.5.0 is technically a semver major release, and as such can include breaking changes. We highly recommend pinning your dependency to prevent future breaking changes from affecting your workflow.

A future version will migrate to using sticky-pull-request-comment, which will no longer require that we manually embed an identifier into the comment. You can upgrade right now by replacing the Find, Create, and Update steps with the following:

- name: Comment
  uses: marocchino/sticky-pull-request-comment@v2
  with:
    header: next-touched-pages
    message: ${{ steps.get-comment-body.outputs.body }}

[jzxhuang]

@dstaley awesome, thanks for the quick update! I didn't realize adding the dep would also affect what the GH action pulled, but I guess that makes a lot of sense. Maybe consider adding a short blurb to the README recommending folks to pin the version of the script as part of the installation steps? 🤷

The sticky-pull-request-comment action looks dope :)

[gaearon]

@dstaley

Update all instances of npx -p nextjs-bundle-analysis in your workflow file to use an explicit version (npx -p nextjs-bundle-analysis@0.4.0).

Shouldn't this be done directly in https://github.com/hashicorp/nextjs-bundle-analysis/blob/main/template.yml? If people copy it (like we did), it's really easy to miss that it opts you into breaking changes. I would suggest to change the template to use a fixed version, and then bump it up manually as you do new releases.

[dstaley]

I would suggest to change the template to use a fixed version, and then bump it up manually as you do new releases.

@gaearon We're discussing internally which of the two approaches we want to default to, and knowing why you preferred one over the other would be helpful! Would you mind sharing what motivated you to go with this approach in lieu of adding nextjs-bundle-analysis to your devDependencies which would pin the version (option 1 above)?

For some context, pinning the version in the workflow file as option 2 describes circumvents dependency analysis tools like Dependabot and npm outdated. It requires that users manually go in and adjust the version of the dependency used in the workflow file, and opens us up to the possibility to shipping a version mismatch by mistake.

However, option one is not without its drawbacks, the most significant of which is a user potentially missing the npm install -D nextjs-bundle-analysis step. (I think this can be worked around by using npx --no in the workflow file though).

[votemike]

Is there any update on this?
The new version was mentioned in April.