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

Enhancement/automated pr wprocket and remove dist #12

Merged

Conversation

MathieuLamiot
Copy link
Contributor

Description

Over the past 2 weeks, the use of this repository has been complicated as it is not clear for everyone how to use it to update the script.
This PR fixes a few practices that could help: the dist files are not stored anymore, but build when needed ; when releasing a new version, a PR is opened on WP Rocket directly to manage the transition to wp-rocket repo.

The new GH action needs tokens and a branch on WP Rocket.

Documentation

User documentation

  • On merge to trunk, the package is distributed to npmjs with dynamically built files. then a PR is opened on WP Rocket with the new files and updated dependency in package.json.
  • dist files are not stored in GH anymore.

Technical documentation

NA

Type of change

Delete options that are not relevant.

  • New feature (non-breaking change which adds functionality).
  • Bug fix (non-breaking change which fixes an issue).
  • Enhancement (non-breaking change which improves an existing functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as before).

New dependencies

NA

Risks

The new GH action will be need to be tested.

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.

Documentation

  • I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
  • The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
  • I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

  • I wrote self-explanatory code about what it does.
  • I wrote comments to explain why it does it.
  • I named variables and functions explicitely.
  • I protected entry points against unexpected inputs.
  • I did not introduce unecessary complexity.
  • I listed the introduced external dependencies explicitely on the PR.
  • I validated the repo-specific guidelines from CONTRIBUTING.md.

Observability

  • I prepared ways to observe the implemented system (logs, data, etc.).

Risks

  •  I explicitely mentioned performance risks in the PR.
  • I explicitely mentioned security risks in the PR.

@wordpressfan
Copy link
Collaborator

@MathieuLamiot

Good point, I just want to share a small thing with you.
This will work perfectly with release but during development phase we were using the rocket-scripts branch name in github instead of the release version in that case when dist directory is not there, we will not be able to copy those js files into WP Rocket assets.
what do u think?

@MathieuLamiot
Copy link
Contributor Author

Thanks for the feedback @wordpressfan
I am not sure to understand, sorry 😬 In both actions, there is a npm run build to generate the dist/ files before publishing or sending to WP Rocket so that should be covered.
If you are talking about manually importing from rocket-scripts into a WP Rocket version to test during development phase, then yes you are right. The dev would have to run npm run build in rocket-scripts first to get access to the files. Is that an issue?

@wordpressfan
Copy link
Collaborator

In both actions, there is a npm run build to generate the dist/ files before publishing or sending to WP Rocket so that should be covered.

Yes this would work with the release workflow and that's great.

If you are talking about manually importing from rocket-scripts into a WP Rocket version to test during development phase, then yes you are right. The dev would have to run npm run build in rocket-scripts first to get access to the files. Is that an issue?

How would they do that? u mean opening the branch they want in rocket-scripts and running build script then copying those files manually to WPR?
When using the branch name inside package.json in WPR, we get the files from git and now we don't have dist in git so we have to move them manually, that's the case that I'm talking about.

@MathieuLamiot
Copy link
Contributor Author

MathieuLamiot commented Aug 14, 2024

Ah right, got it now. Hmm let me think :D

@MathieuLamiot
Copy link
Contributor Author

MathieuLamiot commented Aug 14, 2024

We could adjust wp-rocket to build wp-rocket-scripts intead of just copying the dist files?

We could add a npm script in package.json:
"build:js:beacon": "cd node_modules/wp-rocket-scripts && npm install && npm run build && cd ../.. && gulp copy:js:beacon",

So that, after npm install, instead of doing npm run gulp build:js:beacon, we do npm run build:js:beacon. The script will build the dependency and copy the files. WDYT?

Seems to be working on my end

@wordpressfan
Copy link
Collaborator

I was thinking of something like that but with postinstall hook
https://docs.npmjs.com/cli/v10/using-npm/scripts#npm-install

So once we run npm install we can check if the dist directory is not there so go to that directory and run the build command there so the files will be ready to be used by our WPR gulp task anytime, otherwise do nothing as dist is there already.

wordpressfan
wordpressfan previously approved these changes Aug 16, 2024
Copy link
Collaborator

@wordpressfan wordpressfan left a comment

Choose a reason for hiding this comment

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

We need to create a followup PR in WPR repo as mentioned in the comments above.

@wordpressfan wordpressfan marked this pull request as ready for review August 16, 2024 09:44
@MathieuLamiot MathieuLamiot merged commit 4691b42 into develop Aug 16, 2024
1 of 2 checks passed
@wordpressfan wordpressfan deleted the enhancement/automated-pr-wprocket-and-remove-dist branch August 16, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants