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 local resource error on browser (#392) #394

Merged
merged 2 commits into from Aug 25, 2022
Merged

Fix local resource error on browser (#392) #394

merged 2 commits into from Aug 25, 2022

Conversation

Willy-JL
Copy link
Contributor

@Willy-JL Willy-JL commented Aug 23, 2022

Using import.meta.url leads to bugs on browser since building will hardcode those as local file:// urls. This fix uses document.location instead.

In particular this fixes #392 which made impossible specifying a custom corePath on the same domain.

EDIT: this is now a more appropriate fix, import.meta.url is parsed instead of being hardcoded and webpack has been bumped in order to support this

@jeromewu
Copy link
Collaborator

Hmm, not so sure about this as we just started to use import.meta.url: #341

Maybe @nxtexe can provide some suggestions. 😃

@nxtexe
Copy link
Contributor

nxtexe commented Aug 24, 2022

Not sure what you mean here. import.meta.url AFAIK exposes info on the current context of the executing script. Could you offer a bit more info? Maybe there's something I'm not getting here.

@Willy-JL
Copy link
Contributor Author

Willy-JL commented Aug 24, 2022

Not sure what you mean here. import.meta.url AFAIK exposes info on the current context of the executing script. Could you offer a bit more info? Maybe there's something I'm not getting here.

I may just be doing something wrong since js and webdev were never my thing so I'm trying to learn as I go, but what I observed is that building the source into a .min.js will replace all "import.meta.url" with its value at compile time, so when running from browser it will have the import url hardcoded as "file://path/to/build/environment", which would be useless on all other systems since only the system that built the .min.js has that path, and also browsers don't allow direct file:// access at all.

I can see how import.meta.url would be a good fit for the node side of things, it allows you to specify a corePath relative to where you are importing from, but in browser this makes it impossible to import on a relative path on the same domain:

While the source shows:

coreRemotePath = new URL(_corePath, import.meta.url)

The build process will hardcode "import.meta.url" at compile time, so the browser effectively runs:

coreRemotePath = new URL(_corePath, "file:///path/to/build/environment")

So say my html is on localhost:3000/example.html, I have the core at localhost:3000/static/js/ffmpeg-core.js, the browser would end up running:

coreRemotePath = new URL("static/js/ffmpeg-core.js", "file:///path/to/build/environment")
console.log(coreRemotePath)
// file:///path/to/build/environment/static/js/ffmpeg-core.js

So even though I am running from a website on a web server, it tries to get the file locally instead of relatively to the path on the web server.

Maybe there's a way to add support for import.meta.url when building, but to be honest importing seems more related to the node side of things as opposed to minified browser scripts... that's why my changes only affected the src/browser directory, with node it makes sense to use the import url but for browser it breaks the custom corePath feature at the moment.

@nxtexe
Copy link
Contributor

nxtexe commented Aug 24, 2022

Well I see what you mean and you have a point. Referencing this issue it seems it's a webpack issue. It seems webpack is statically replacing references of import.meta.url; which really shouldn't be the case. Not sure right now if there is a way to circumvent this behaviour while keeping support for web workers. I say this because the reason import.meta.url was chosen in the first place was because the document interface is not available in worker contexts. I'll do a bit more digging and offer an update in the coming days.

@Willy-JL
Copy link
Contributor Author

Willy-JL commented Aug 24, 2022

Seems like we aren't the only ones with this issue 😄

webpack/webpack#15246

It is indeed a sort of compile flag

Will fix my PR in a bit

@Willy-JL
Copy link
Contributor Author

Ok so it works and it doesnt, and im not so happy with this fix to be honest. Now it doesnt hardcode the local path, but the issue is that when importing the script into html it must be marked as type="module", and subsequently all other scripts that want to use ffmpeg must be marked as type="module"...

@jeromewu
Copy link
Collaborator

Got it, maybe we can add some description in the README.md to address the usage? That should be helpful. 😄

@Willy-JL
Copy link
Contributor Author

Willy-JL commented Aug 25, 2022

Agreed. I personally got around my issue by passing the full path including domain by creating the url dynamically on my side:

const ffmpeg = createFFmpeg({
  ...,
  corePath: new URL("static/js/ffmpeg-core.js", document.location).href,
});

That could be an example perhaps... this way the user can handle it themselves and we don't have issues library wide

@Willy-JL
Copy link
Contributor Author

Got it, maybe we can add some description in the README.md to address the usage? That should be helpful. smile

Perhaps like that?

@jeromewu
Copy link
Collaborator

LGTM! Thanks for the PR!

@jeromewu jeromewu merged commit 2a181ee into ffmpegwasm:master Aug 25, 2022
@Willy-JL Willy-JL deleted the fix-local branch August 26, 2022 01:18
jeromewu added a commit that referenced this pull request Aug 10, 2023
Fix local resource error on browser (#392)
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.

Cannot specify a custom corePath on same domain
3 participants