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

Move main from index.js to BeaconEntryPoint #19

Merged
merged 5 commits into from
Aug 20, 2024

Conversation

MathieuLamiot
Copy link
Contributor

Description

Follow-up of #12 and needed for auto build in WP Rocket: we don't use index.js anymore as the entry point, but we use BeaconEntryPoint.js. So a few things must be adapted so that npm knows it and install the right files.

Documentation

User documentation

NA

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

NA

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.
  • I implemented built-in tests to cover the new/changed code.

Copy link

codacy-production bot commented Aug 16, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for ae446361 100.00% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (ae44636) Report Missing Report Missing Report Missing
Head commit (95fa550) 409 241 58.92%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#19) 3 3 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

@wordpressfan
Copy link
Collaborator

@mostafa-hisham will this work with inspector code without a problem?

@MathieuLamiot
Copy link
Contributor Author

Let's discuss this with @mostafa-hisham tomorrow. Maybe inspector will need an adaptation as well. ; but we need to either have the index.js, either the EntryPoint but we shouldn't keep both approaches I think.

@mostafa-hisham
Copy link
Contributor

mostafa-hisham commented Aug 20, 2024

@MathieuLamiot we have to export the class that will be used in wpr-inspector
I think the class with the logic now is BeaconManager so we have to export it then import the class in the inspector.

One more thing I did in wpr-inspector , I overwrote some functions (save and get stuff from WP DataBase) in the LCPBeacon so I can use it "in test mode"

https://github.com/wp-media/wpr-inspector/blob/5c0b5adfb08dbbe207dc80d2d5704e859a826cc9/wpr-inspector/LCPTester.js#L94-L111

I see all functions are still the same in BeaconManager expect _isGeneratedBefore

  • ✔️ _saveFinalResultIntoDB
  • ✔️ _finalize
  • ✔️ _handleInfiniteLoop
  • ✖️ _isGeneratedBefore is now _getGeneratedBefore should i make the new function return false?

Another idea we can create an inspector mode in this repro BeaconManager that do what is needed to run it in wpr-inspector and use BeaconManager in the inspector mode by setting a config, instead of overwriting the class in wpr-inspector

@MathieuLamiot
Copy link
Contributor Author

_isGeneratedBefore is now _getGeneratedBefore should i make the new function return false?

Yes, I think so.

Another idea we can create an inspector mode in this repro BeaconManager that do what is needed to run it in wpr-inspector and use BeaconManager in the inspector mode by setting a config, instead of overwriting the class in wpr-inspector

I don't know if that would be better 🤔 Those hacks are quite specific to the inspector, so I am not sure it would make sense to make them in the shared codebase?

we have to export the class that will be used in wpr-inspector
I think the class with the logic now is BeaconManager so we have to export it then import the class in the inspector.

Does that mean adding export default BeaconManager; to BeaconEntryPoint.js, if this file is now used as main on rocket-scripts? I don't know how it would interact on both side (inspector and wp-rocket).
@mostafa-hisham, can you have a look into this? Feel free to commit on those PR to adjust. Thank you

@MathieuLamiot MathieuLamiot merged commit cba54a8 into develop Aug 20, 2024
4 checks passed
@wordpressfan wordpressfan deleted the integration/wp-rocket-auto-build branch August 20, 2024 12:32
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.

3 participants