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

Revert recent changes resulting in problems with mix-manifest.json #68

Merged
merged 1 commit into from
Sep 12, 2020

Conversation

simensen
Copy link
Contributor

This is only triage to get Laravel Mix working again. Additional work may be required to get Livewire's manifest.json working correctly.

This reverts #67 f2e34ab which was an attempt to fix #35 758214a.

The end result of #67 was mix-manifest.json being included in the asset artifact and being removed from the code deployment artifact. This meant Laravel Mix applications would fail as they expect to be able to read the contents of public/mix-manifest.json at runtime.

This does not attempt to fix the original issue #35 was trying to address. #35 would have included public/manifest.json in the code deployment artifact (the desired behavior) but #67 would ensure both public/manifest.json and public/mix-manifest.json would end up in the asset artifact.

Unclear if Livewire's manifest.json is needed by both client (so needs to be a part of the asset artifact) and code (so also needs to be in the code deployment artifact) so not trying to make that decision now.


See this comment on #67 regarding my proposal for solving this problem in a more generic way by allowing people to specify which public assets should be included only in the code deployment artifact or also in the code deployment artifact.

TL;DR: For example, allowing vapor.yml to support specifying paths and patterns like this:

public_assets:
    # Files that are only needed by code and can be removed from
    # the asset artifact.
    only_in_code_artifact:
        - mix-manifest.json
    # Files that are needed by BOTH the code and also need to be
    # included in the asset artifact
    also_in_code_artifact:
        - manifest.json
        - images/logos/*.svg
        - images/icons/*.svg

This is only triage to get Laravel Mix working again. Additional work
may be required to get Livewire's `manifest.json` working correctly.

This reverts laravel#67 f2e34ab which was an
attempt to fix laravel#35 758214a.

The end result of laravel#67 was mix-manifest.json being included in the asset
artifact and being removed from the code deployment artifact. This meant
Laravel Mix applications would fail as they expect to be able to read
the contents of `public/mix-manifest.json` at runtime.

This does not attempt to fix the original issue laravel#35 was trying to
address. laravel#35 would have included `public/manifest.json` in the code
deployment artifact (the desired behavior) but laravel#67 would ensure both
`public/manifest.json` and `public/mix-manifest.json` would end up in
the asset artifact.

Unclear if `manifest.json` is needed by both client (so needs to be a
part of the asset artifact) and code (so also needs to be in the code
deployment artifact) so not trying to make that decision now.
@taylorotwell taylorotwell merged commit e7ca5b0 into laravel:master Sep 12, 2020
@harrygulliford
Copy link
Contributor

The file generated and required by Livewire at runtime is manifest.json (see here). I don't believe this PR would include that file?

I suggest using *manifest.json or reverting back to *.json for now. It is pretty important that this is fixed as Livewire is now coupled with Jetstream.

@simensen
Copy link
Contributor Author

@harrygulliford Can you confirm for me whether manifest.json is used for both client and code, or is it only used for code?

@simensen
Copy link
Contributor Author

@harrygulliford I've just submitted #70 to fix this specifically for Laravel Livewire. However, I don't know enough about Livewire to know for sure if that PR will actually fix it. It is clear that Laravel itself needs to read public/manifest.json in the code deployment artifact but I don't know if the client needs to download public/manifest.json as well.

.oO( ... grumbles something about people writing code-side required files into public/ when they don't need to be downloaded by the client... )

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