feat: Hot Reloading #7489

lonnieezell · 2023-05-10T05:14:17Z

This adds a new "Hot Reload" feature to the debug toolbar. When active, it will scan the app directory for any changes to files and instruct the browser to reload if any changes were found.

I still need to write tests, but submitting it so that others can try it out first and see if you encounter any problems. I've seen at least one strange thing but unsure if it is just my setup or a more widespread problem.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

michalsn

Looks good!

I gave it a try on:

Firefox, Chrome, and Safari on macOS (with nginx)
Firefox, Chrome, and Edge on Windows (with Apache)

Besides some general comments on the code, I noticed some visual issues when hot reloading is enabled:

hot-reload-visual-issue.mp4

Please take a look at the vertical scroll bar on the toolbar, which is moving while the icon is spinning.

system/HotReloader/HotReloader.php

system/Debug/Toolbar/Views/toolbar.js

system/HotReloader/HotReloader.php

lonnieezell · 2023-05-15T04:31:22Z

@michalsn Thanks for these comments! Good stuff, and exactly why I was wanting other eyes on this. I had missed the scrollbar wackiness. Maybe having the animation is more trouble than it is worth but I'll take another look at it.

iRedds · 2023-05-15T05:20:11Z

If this feature is associated with a tool bar, then it must be in the CodeIgniter\Debug namespace.
Should only be enabled if the toolbar is enabled.

lonnieezell · 2023-05-15T13:39:11Z

If this feature is associated with a tool bar, then it must be in the CodeIgniter\Debug namespace.

I disagree. This is no more a toolbar feature than any of the other info panes are associated with the toolbar. There is a button on the toolbar that starts and stops it, but that is as far as the association goes.

Should only be enabled if the toolbar is enabled.

I can operate without the toolbar if someone wanted to implement just a few lines of JS on their site. There's nothing that would stop it from working. However, in practice it does operate only when the toolbar is enabled, since it's currently the only way to start it. If the toolbar is disabled during while the hot reloader is active then the connection would be closed on page refresh.

michalsn

Thanks, @lonnieezell - now it's working great!

Although there is still a small problem with this icon. True, the slider does not appear now, but the icon moves slightly up and down. This is probably a problem with selecting the wrong element in the DOM. We should be working on the img element, not a. I have added a code suggestion to solve this.

system/Debug/Toolbar/Views/toolbar.tpl.php

system/Debug/Toolbar/Views/toolbar.js

lonnieezell · 2023-06-24T05:04:08Z

Why is it so difficult to get a PR to a place to make our 25 checks happy. This is so frustrating to work with anymore.

PHP Linter is failing due to stuff that has nothing to do with this PR. I rebased against the current 4.4 branch and they're still there.

I realized that I had put my CSS changes in the CSS file, forgetting that we had a SASS file in place, so I moved that change, ran dart sass locally as explained in our contributing docs, and all worked fine, but the tests are failing here.

Why are we being forced to add @extends / @implements into the docblocks? How is this helpful at all? I'm actually curious. I've been programming PHP for 15 years and never needed it. It seems pretty obvious by looking at the class declaration what it extends and implements. What is the benefit? And what is the right lines to use in the IteractorFilter because I obviously don't have a clue.

lonnieezell · 2023-06-24T05:05:04Z

system/HotReloader/IteratorFilter.php

+/**
+ * @internal
+ *
+ * @extends RecursiveFilterIterator<RecursiveIterator>


@paulbalandan @kenjis What's the proper lines to have in the docblock here to make our static analysis happy?

Something like this? But I don't know the exact types.

* @template-extends RecursiveFilterIterator<TKey, TValue, TIterator> * @template-implements RecursiveIterator<TKey, TValue>

There is some question as to how useful this type information is, so we can suppress errors.

--- a/system/HotReloader/IteratorFilter.php +++ b/system/HotReloader/IteratorFilter.php @@ -17,8 +17,7 @@ use RecursiveIterator; /** * @internal * - * @extends RecursiveFilterIterator<RecursiveIterator> - * @implements RecursiveIterator<RecursiveIterator> + * @psalm-suppress MissingTemplateParam */ final class IteratorFilter extends RecursiveFilterIterator implements RecursiveIterator {

Thank you, that works.

kenjis · 2023-06-24T07:17:01Z

PHP Linter is failing due to stuff that has nothing to do with this PR. I rebased against the current 4.4 branch and they're still there.

Rebase and composer update will solve the error.

kenjis · 2023-06-24T08:08:14Z

Why are we being forced to add @extends / @implements into the docblocks? How is this helpful at all? I'm actually curious. I've been programming PHP for 15 years and never needed it. It seems pretty obvious by looking at the class declaration what it extends and implements. What is the benefit? And what is the right lines to use in the IteractorFilter because I obviously don't have a clue.

It is generics. It provide more type information that cannot express PHP syntax.
https://psalm.dev/docs/annotating_code/templated_annotations/

When removing all tags:

ERROR: MissingTemplateParam - system/HotReloader/IteratorFilter.php:19:13 - CodeIgniter\HotReloader\IteratorFilter has missing template params when extending RecursiveFilterIterator, expecting 3 (see https://psalm.dev/182)
final class IteratorFilter extends RecursiveFilterIterator implements RecursiveIterator


ERROR: MissingTemplateParam - system/HotReloader/IteratorFilter.php:19:71 - CodeIgniter\HotReloader\IteratorFilter has missing template params when extending RecursiveIterator, expecting 2 (see https://psalm.dev/182)
final class IteratorFilter extends RecursiveFilterIterator implements RecursiveIterator


ERROR: MissingTemplateParam - system/HotReloader/IteratorFilter.php:21:71 - CodeIgniter\HotReloader\IteratorFilter has missing template params when extending RecursiveIterator, expecting 2 (see https://psalm.dev/182)
final class IteratorFilter extends RecursiveFilterIterator implements RecursiveIterator


ERROR: MissingTemplateParam - system/HotReloader/IteratorFilter.php:32:13 - CodeIgniter\HotReloader\IteratorFilter has missing template params when extending RecursiveFilterIterator, expecting 3 (see https://psalm.dev/182)
final class IteratorFilter extends RecursiveFilterIterator implements RecursiveIterator

kenjis · 2023-06-24T08:24:12Z

What is the benefit?

Yeah, it is certainly too complicated to know what it is.

https://github.com/vimeo/psalm/blob/81946bbbc39b3e5c688815b58c905d7d8d64146e/stubs/CoreGenericIterators.phpstub#L679-L687

https://github.com/vimeo/psalm/blob/81946bbbc39b3e5c688815b58c905d7d8d64146e/stubs/CoreGenericIterators.phpstub#L67C1-L73

kenjis · 2023-06-24T08:44:45Z

I realized that I had put my CSS changes in the CSS file, forgetting that we had a SASS file in place, so I moved that change, ran dart sass locally as explained in our contributing docs, and all worked fine, but the tests are failing here.

Running sass will update toolbar.css. Did you update sass?

$ sass --no-source-map admin/css/debug-toolbar/toolbar.scss system/Debug/Toolbar/Views/toolbar.css

$ git diff
diff --git a/system/Debug/Toolbar/Views/toolbar.css b/system/Debug/Toolbar/Views/toolbar.css
index 60c3216bfc..38bab087ae 100644
--- a/system/Debug/Toolbar/Views/toolbar.css
+++ b/system/Debug/Toolbar/Views/toolbar.css
@@ -7,327 +7,321 @@
  * file that was distributed with this source code.
  */
 #debug-icon {
-    bottom: 0;
-    position: fixed;
-    right: 0;
-    z-index: 10000;
-    height: 36px;
-    width: 36px;
-    margin: 0px;
-    padding: 0px;
-    clear: both;
-    text-align: center;
+  bottom: 0;
+  position: fixed;
+  right: 0;
+  z-index: 10000;
+  height: 36px;

Co-authored-by: Michal Sniatala <michal@sniatala.pl>

…i_set

lonnieezell · 2023-06-25T05:00:17Z

@kenjis Thanks for your help. I was pretty frustrated last night. I feel like every time I have a contribution I spend as long trying to appease the tools as I do working on the feature itself. It always seems to happen that you fix one tooling issue and more crop up.

And, while I understand what generics are, my usage of them is limited to within Typescript when needed.

But I just have to stop and think that if I find this so challenging to work with at times, I can't imagine what non-core contributors feel when they want to contribute and how many PRs this whole process has stopped from happening.

lonnieezell · 2023-06-25T05:34:39Z

And it's all green!

user_guide_src/source/testing/debugging.rst

user_guide_src/source/changelogs/v4.4.0.rst

kenjis · 2023-06-25T08:50:11Z

I tested this feature a little with spark serve.
It seems to work fine.

michalsn

🚀

Co-authored-by: kenjis <kenji.uui@gmail.com>

lonnieezell · 2023-06-26T04:41:51Z

See - I make changes to some user guide pages and suddenly Rector has issues in the RequestTrait that it didn't previously have.

/sigh.

Hopefully we don't need to address that for this and can get this merged.

datamweb · 2023-06-26T04:44:09Z

I make changes to some user guide pages and suddenly Rector has issues in the RequestTrait that it didn't previously have.

This is not related to this PR.

kenjis · 2023-06-26T05:41:05Z

Don't worry. Rector check fails now. See #7606

lonnieezell · 2023-06-26T13:03:41Z

Thanks for merging!

ALTITUDE-DEV-FR · 2023-08-25T13:19:54Z

Excellent feature thanks !!!

lonnieezell · 2023-08-25T13:39:45Z

Glad you like it!

ALTITUDE-DEV-FR · 2023-08-25T13:42:16Z

Glad you like it!

i have small problem with this.
EventSource's response has a MIME type ("text/html") that is not "text/event-stream". Aborting the connection.

lonnieezell self-assigned this May 10, 2023

lonnieezell added new feature PRs for new features 4.4 labels May 10, 2023

michalsn reviewed May 10, 2023

View reviewed changes

system/HotReloader/HotReloader.php Outdated Show resolved Hide resolved

system/Debug/Toolbar/Views/toolbar.js Outdated Show resolved Hide resolved

system/HotReloader/HotReloader.php Show resolved Hide resolved

michalsn reviewed May 17, 2023

View reviewed changes

system/Debug/Toolbar/Views/toolbar.tpl.php Outdated Show resolved Hide resolved

system/Debug/Toolbar/Views/toolbar.js Outdated Show resolved Hide resolved

lonnieezell changed the title ~~Draft: feat: Hot Reloading~~ feat: Hot Reloading May 23, 2023

lonnieezell commented Jun 24, 2023

View reviewed changes

lonnieezell and others added 15 commits June 24, 2023 23:53

feat: Hot Reloading

aba163e

Keep nginx from sending 504 time-out errors

3840990

Co-authored-by: Michal Sniatala <michal@sniatala.pl>

fix: Keep scrollbar from showing up due to rotating icon

a22a8c1

fix: Remove output_buffering call that cannot actually be set with in…

8be1d5f

…i_set

fix: Make the hot reload script work for subfolder hosting.

caeaf5d

Add tests for Directory Hasher.

aa800a9

Added michalsn's suggestion to rotate the image not the anchor.

470bcb9

fix: attempting to fix style, user guide, and other small errors

d2c8e45

fix: attempting to fix style, user guide, and other small errors

c6b6341

fix: additional code style fix

2b52354

More style fixes

4463200

Rector updates

884a91e

Run cs-fix

0bca646

CSS corrections, and other utility fixes

3fd7e4b

Attempting to make psalm happy

a1bbe2d

lonnieezell added 2 commits June 24, 2023 23:53

Run sass converter to generate toolbar.css

7fd7cca

Suppress Psalm errors on IteratorFilter

b1a2e1d

lonnieezell force-pushed the hot-reload branch from a8f2cb8 to b1a2e1d Compare June 25, 2023 04:55

Add missing group to test

fc5bd02

kenjis reviewed Jun 25, 2023

View reviewed changes

user_guide_src/source/testing/debugging.rst Show resolved Hide resolved

kenjis reviewed Jun 25, 2023

View reviewed changes

user_guide_src/source/testing/debugging.rst Outdated Show resolved Hide resolved

kenjis reviewed Jun 25, 2023

View reviewed changes

user_guide_src/source/testing/debugging.rst Outdated Show resolved Hide resolved

kenjis reviewed Jun 25, 2023

View reviewed changes

user_guide_src/source/changelogs/v4.4.0.rst Outdated Show resolved Hide resolved

michalsn approved these changes Jun 25, 2023

View reviewed changes

lonnieezell and others added 2 commits June 25, 2023 23:34

Apply suggestions from code review

ac761f0

Co-authored-by: kenjis <kenji.uui@gmail.com>

Categorize the changelog entry for hot reloading

3b67f49

datamweb approved these changes Jun 26, 2023

View reviewed changes

kenjis approved these changes Jun 26, 2023

View reviewed changes

kenjis merged commit 2c1ce59 into 4.4 Jun 26, 2023

kenjis deleted the hot-reload branch June 26, 2023 05:42

kenjis mentioned this pull request Mar 27, 2023

[4.4] Important messages change codeigniter4/translations#384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Hot Reloading #7489

feat: Hot Reloading #7489

lonnieezell commented May 10, 2023

michalsn left a comment

lonnieezell commented May 15, 2023

iRedds commented May 15, 2023

lonnieezell commented May 15, 2023

michalsn left a comment

lonnieezell commented Jun 24, 2023

lonnieezell Jun 24, 2023

kenjis Jun 24, 2023

lonnieezell Jun 25, 2023

kenjis commented Jun 24, 2023

kenjis commented Jun 24, 2023

kenjis commented Jun 24, 2023

kenjis commented Jun 24, 2023

lonnieezell commented Jun 25, 2023

lonnieezell commented Jun 25, 2023

kenjis commented Jun 25, 2023

michalsn left a comment

lonnieezell commented Jun 26, 2023

datamweb commented Jun 26, 2023 •

edited

Loading

kenjis commented Jun 26, 2023

lonnieezell commented Jun 26, 2023

ALTITUDE-DEV-FR commented Aug 25, 2023

lonnieezell commented Aug 25, 2023

ALTITUDE-DEV-FR commented Aug 25, 2023

feat: Hot Reloading #7489

feat: Hot Reloading #7489

Conversation

lonnieezell commented May 10, 2023

michalsn left a comment

Choose a reason for hiding this comment

lonnieezell commented May 15, 2023

iRedds commented May 15, 2023

lonnieezell commented May 15, 2023

michalsn left a comment

Choose a reason for hiding this comment

lonnieezell commented Jun 24, 2023

lonnieezell Jun 24, 2023

Choose a reason for hiding this comment

kenjis Jun 24, 2023

Choose a reason for hiding this comment

lonnieezell Jun 25, 2023

Choose a reason for hiding this comment

kenjis commented Jun 24, 2023

kenjis commented Jun 24, 2023

kenjis commented Jun 24, 2023

kenjis commented Jun 24, 2023

lonnieezell commented Jun 25, 2023

lonnieezell commented Jun 25, 2023

kenjis commented Jun 25, 2023

michalsn left a comment

Choose a reason for hiding this comment

lonnieezell commented Jun 26, 2023

datamweb commented Jun 26, 2023 • edited Loading

kenjis commented Jun 26, 2023

lonnieezell commented Jun 26, 2023

ALTITUDE-DEV-FR commented Aug 25, 2023

lonnieezell commented Aug 25, 2023

ALTITUDE-DEV-FR commented Aug 25, 2023

datamweb commented Jun 26, 2023 •

edited

Loading