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

Bundle three.js examples in module build #238

Merged
merged 1 commit into from
Jun 14, 2021
Merged

Bundle three.js examples in module build #238

merged 1 commit into from
Jun 14, 2021

Conversation

mskelton
Copy link
Contributor

Description

Fixes #188

Specific Changes proposed

Issue #188 describes an issue where using the module build (videojs-vr.es.js) will break with the error THREE is not defined. This is because the three.js example files are not bundled and so module consumers receive the unchanged version when installing three.js from npm.

This PR adds a custom resolver which will resolve the three.js example files and mark them as non-external. This matches the output from the commonjs bundle where the examples are part of the output bundle and thus the changes from rollup replace are preserved.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Unit Tests updated or fixed
    • Docs/guides updated
  • Reviewed by Two Core Contributors

@mskelton
Copy link
Contributor Author

@brandonocasey @gkatsev Could we get some eyes on this PR? It fixes this library for users using a package manager (e.g. npm, Yarn, pnpm) as the library is currently broken for package manager users. Issue #188 explains the issue that is happening and the PR description of this PR should explain how this PR fixes it.

Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. Thanks for the pull request, sorry it took us so long to get to it.

@brandonocasey brandonocasey merged commit 93bec48 into videojs:master Jun 14, 2021
@mskelton mskelton deleted the rollup-resolve branch June 14, 2021 16:07
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.

THREE is not defined
2 participants