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

Fix incorrect outputFileName slash on Windows #12

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

sxxov
Copy link
Contributor

@sxxov sxxov commented Nov 16, 2020

When building on Windows, there'll be an error in the browser when trying to import the generated module:

Uncaught TypeError: Failed to construct 'URL': Invalid URL

This is because path.join uses backslashes on Windows instead of forward ones that new URL() expects:

// error
new URL('foo\bar', import.meta.url);

// all good
new URL('foo\bar'.replace(/\\/g, '/'), import.meta.url);

@malchata malchata merged commit 7757fab into malchata:master Mar 2, 2021
@sxxov
Copy link
Contributor Author

sxxov commented Mar 2, 2021

@malchata I am a better man since i wrote that PR.

The correct way to fix it would be to replace path.join with path.posix.join rather than do it with regex

@malchata
Copy link
Owner

malchata commented Mar 2, 2021

I deeply apologize for the delay in merging this @sxxov. I had some watch settings for repositories that were burying notifications for this repo and it slipped.

I published an update to this package you may use to test. To ensure I don't unintentionally break dependent applications through a normal install, I published a version containing these (and other) proposed changes to the package's next tag, which you can install as npm i rollup-plugin-imagemin@next. Once you (and other PR submitters) are satisfied, I will republish the package without the next tag.

Please also note that, due to a recently added warning in Rollup, the default export has been dropped and is now a single named export named imagemin. You can check the README for exact usage details.

@malchata
Copy link
Owner

malchata commented Mar 2, 2021

Ack, you slipped in a comment while I was writing mine!

If you want to submit another PR to use that instead, I'd be happy to merge it. :)

@sxxov
Copy link
Contributor Author

sxxov commented Mar 2, 2021

@malchata Hey no problem with the PR accepting things, shit happens

I'll test it out when I get the chance to, especially thanks for the rollup heads-up too!

@malchata
Copy link
Owner

malchata commented Mar 2, 2021

No worries, and just a heads up: I changed the default branch from master to main, so you may need to update your local or re-clone.

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.

2 participants