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.

Sign up for GitHub

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

Jump to bottom

Fix JS path with excedent slash #3280

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

Fix JS path with excedent slash #3280

wants to merge 1 commit into from

Conversation

mrleblanc101
Copy link

Fix #1717

@thecrypticace
Copy link
Collaborator

@mrleblanc101 I don't understand why this is necessary. Can you provide a reproduction webpack.mix.js file, what the output is, and what you expect it to be?

@mrleblanc101
Copy link
Author

Did you look at the linked issue ? If it's still not clear, I'll gladly open a repo

@thecrypticace
Copy link
Collaborator

I did but I'm still a little confused.

@mrleblanc101
Copy link
Author

mrleblanc101 commented Jun 14, 2022 •
edited

Here is the repo: https://github.com/mrleblanc101/repro-laravel-mix-1717

First, as you can see, the terminal show that the JS has a leading slash but not the CSS.
Capture d’écran, le 2022-06-14 à 16 15 56
If this was just visual I wouldn't mind, but this create an issue in standalone mode.

I'm using this with laravel-mix-twig (using html-webpack-plugin) to create a static site served by BrowserSync.
The problem is that the JS path is incorrect. It has a additionnal leading slash.

Capture d’écran, le 2022-06-14 à 18 08 04

Capture d’écran, le 2022-06-14 à 18 08 01

Capture d’écran, le 2022-06-14 à 18 09 57

I was able to mix.webpackConfig to bypass this, but while it work, it's still wrong.

mix.webpackConfig({
  output: {
    publicPath: ".",
  },
});

will output

<head>
    <script defer src=".//js/app.js"></script>
    <link href="./css/app.css" rel="stylesheet">
</head>

or

mix.webpackConfig({
  output: {
    publicPath: "",
  },
});

will output:

<head>
    <script defer src="/js/app.js"></script>
    <link href="css/app.css" rel="stylesheet">
</head>

I'm pretty sure from my digging that it's not an issue with laravel-mix-twig, html-webpack-plugin or browser-sync

@mrleblanc101
Copy link
Author

I tried to update the test but I was not able to run ava --update-snapshots

@mrleblanc101
Copy link
Author

mrleblanc101 commented Jun 15, 2022 •
edited

@thecrypticace I see you approved the workflow run. Thanks !
All test could pass without change, we only need to update the snapshots for 5 of them I believe. Any idea how I could do this ? I keep getting error.

@thecrypticace
Copy link
Collaborator

Yeah I wanted to get an idea of what changed. This affects the manifest so I need to test this in a proper Laravel project first before I update the test suite. I won't have time today or tomorrow to look at this but I will try to get to it this week.

@thecrypticace
Copy link
Collaborator

Oh wait no nvm I read the tests wrong. It's just the internal webpack entry that's affected (and things using stats) so yeah updating the tests should be fine. I'll update the snapshots.

@mrleblanc101
Copy link
Author

Ok, thank you. The only reason test are failing is because of this difference between the snapshot and the new output. If we update the snapshot for those test they won't fail anymore.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Honor the public path by removing "/" from generated CSS and JS files
2 participants
@mrleblanc101 @thecrypticace