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

Check unreadable manifest to enable dev mode [CakePHP 4] #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edoardocavazza
Copy link
Contributor

This PR introduces a new development.checkManifest param. When true (defaults to false for backward compatibility) the helper will look for manifest readability in order to detect dev mode.

This is a proposal PR, I'll create the cake 5 version once this has been approved.

@passchn passchn self-assigned this Mar 19, 2024
@passchn passchn added the enhancement New feature or request label Mar 19, 2024
@passchn
Copy link
Collaborator

passchn commented Mar 19, 2024

Hi, thanks for your PR! :)

First, you bring a good point to the table that there should not be an exception happening when first using the plugin without ever have run the vite command (i.e., the manifest.json does not exist). Or do you never have a manifest locally and only bundle assets in production? I'd be interested to hear about your use case/setup! :)

After taking a short look, it seems weird to me that devMode is just assumed if the Manifest does not exist. This should never happen in production, e.g. if there is no manifest for whatever reason.

Also, I don't really like this "exception driven development" where you just use the ManifestNotFound exception in order to detect the app's state. You are just creating the object in runtime and do nothing with it, just look if there is an exception..

I can look into it on the weekend if I find the time. But until then the Exception is the better solution than just assuming you are in dev mode imo.

As far as I remember the error message is also quite clear that a manifest must be created.

@passchn
Copy link
Collaborator

passchn commented Mar 19, 2024

Maybe other people have an opinion on this? @bor-attila @jbennecker

@passchn
Copy link
Collaborator

passchn commented Mar 19, 2024

You could as well look if the file exists in your app's config and set the forceProductionMode config dynamically based on this info. Or use Configure::write(...) somewhere before your View is rendered.

@edoardocavazza
Copy link
Contributor Author

Hi @passchn, thank you for starting a discussion.

Our use case is as follows: without a stringent convention for the use of virtual hosts in local development, the hostNeedles configuration is a bit cumbersome because it needs to be set up by each developer. Sometimes, there are policies from third-party services (like authentication services) that are very strict about using .local or localhost and only work with the official domain name.

Therefore, our need would be to force development mode regardless of the host name.
Initially, I had thought about introducing a forceDevelopmentMode property to be set in the unversioned app_local.php (it's still a valid solution, but I wanted to try a more dynamic solution like the one in question).

Or do you never have a manifest locally and only bundle assets in production? I'd be interested to hear about your use case/setup!

Generally, we do use the dev server, but occasionally, for debugging, we create a local bundle to be more similar to the production environment.

After taking a short look, it seems weird to me that devMode is just assumed if the Manifest does not exist. This should never happen in production, e.g., if there is no manifest for whatever reason.

I totally agree with your objection. I thought it could work as I expect the forceProductionMode to always be set to true for the production environment.

Also, I don't really like this "exception driven development"

Here too, I agree. I was a bit conservative, I didn't want to refactor the ViteManifest class too much to expose a method that would check for the presence of the manifest, but I can revisit this part.


In addition to the forceDevelopmentMode solution, I also thought about a checkDevServer configuration which, instead of checking for the presence of the manifest, would check whether the dev server had started. What's your opinion on this solution?

@bor-attila
Copy link
Contributor

To the current PR:

I think it's ok to assume production mode based on manifest - bcz in prod always exists, and in dev shouldn't.

In worst case/mistake

  • in production you are getting dev mode - lots of 404 links.
  • in dev you are getting the prod mode - your changes will have no effect - just like forgetting forceDevelopmentMode on true

PR is ok, but still not flexible enough.

My solution/idea:

Few weeks ago I ran into a problem. My friend wanted to use dev mode meanwhile his 'boss' checking the prod.

I already forked the repo and I started to rethink all this 'is prodution' or 'is dev' thingy.

So far I removed the forceDevelopmentMode, productionHint and hostNeedles.

I set up an environment config what must be an Enum.

Environment::PRODUCTION - for production

Environment::DEVELOPMENT - for dev

Environment::DETECTOR - if you want to use a detector

With a detector (callback + ServerReqeust param) literally you can solve everything. IP, cookie detection, env variable, path, query, action, etc ...

Sadly my solution is not compatible with the current config/version (maybe after a PR new major version ?!).

@passchn
Copy link
Collaborator

passchn commented Mar 20, 2024

I'm totally fine with rethinking the config and planning a new major. Like 3.x for Cake4 and 4.x for Cake5.

I personally am not using devMode all the time locally. Also, forceProductionMode can be false in production in my case - so I am relying on the development.hostNeedles. I am bundling my assets locally as I don't have a real CI/CD pipeline and I have no nodeJs available on my prod servers. A bit like in the stone age, but it works fine for me :)

I try to think about it and make my own PR which I will link here for discussion.

I think environments might be the clearest way to go. Then, the config keys development.hostNeedles, forceProductionMode and productionHint could be skipped altogether. The config could be set via an Enum provided by the plugin either directly or via a callback where you could use a detector as @bor-attila mentioned.

About the method exposal as @edoardocavazza mentioned: This is depending on the config, because there could e.g. be a manifest from a CakePHP plugin.

The whole config and options handling in the helper is a bit overengineered imo currently.. Maybe this could also be simplified :) It is a bit cumbersome to use this vite plugin within other plugins, I did it here: https://github.com/passchn/cakephp-file-pool/blob/cake5/templates/element/helper/FilePool.php

It works - but maybe the stuff could be simplified.

Anyways, great to hear you are using the plugin actively! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants