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 Vimeo video #172

Closed
wants to merge 5 commits into from
Closed

Fix Vimeo video #172

wants to merge 5 commits into from

Conversation

mgautierfr
Copy link
Contributor

This PR is kind of a fix to #165.

The root issue seems that vimeo is using js module (instead of plain "classic" script) and rewriter is not detecting thing correctly.

If it is somehow easily fixable for inline module, we have to detect that for js resources themselves. We have regex in JS rewriter to detect that, but it seems it is not working. The check

if "module.js" in self.path:
    return "module"
else:
    return "javascript"

works in our use case, but probably (definitively?) not something we want in production.
I'm still searching for a good way to detect that.
For wabac.js, it is easier as js is rewritten as runtime, so it knows the context.

On top of that, the player seems to be broken sometime, depending if you are clicking on the "video background" or the "play" button. Alternate clicking on video/play button launch the video.

@mgautierfr mgautierfr force-pushed the vimeo branch 2 times, most recently from c6dfd45 to ef3a9c4 Compare February 1, 2024 15:40
@mgautierfr mgautierfr changed the title Rename _active_tag to rewrite_context. Fix Vimeo video Feb 1, 2024
Spec says that link to module must start with `./`, `../` or `/`.
A "plain" relative name is not enough, we must make it start with `./`.
How to detect that is still a open question.
@benoit74
Copy link
Collaborator

benoit74 commented May 2, 2024

Superseeded by #228

@benoit74 benoit74 closed this May 2, 2024
@benoit74 benoit74 deleted the vimeo branch May 21, 2024 08:23
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