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

feat(lang): es6 import of a language file #5549

Merged
merged 1 commit into from Nov 5, 2018
Merged

feat(lang): es6 import of a language file #5549

merged 1 commit into from Nov 5, 2018

Conversation

@eranshmil
Copy link
Contributor

@eranshmil eranshmil commented Nov 5, 2018

Description

A temporary solution to enable es6 import of the language files.
Related issue: #5092

How to use:

import es from 'video.js/dist/lang/es.json';

videojs(element, {
  language: 'es',
  languages: {
    es
  }
});

If you use typescript, you need to add the following compilerOptions to the tsconfig:

"resolveJsonModule": true,
"esModuleInterop": true,
"allowSyntheticDefaultImports": true

Specific Changes proposed

Add a grunt task to copy all json files to dist/lang directory.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors
copy all json files to dist/lang directory.

related issue: #5092
@welcome
Copy link

@welcome welcome bot commented Nov 5, 2018

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 5, 2018

This may be a reasonable workaround. Is importing JSON something that's actively supported? Or is that why you mention the need to enable it in TS?

@eranshmil
Copy link
Contributor Author

@eranshmil eranshmil commented Nov 5, 2018

It's supported.
Tested using this boilerplate: https://github.com/metagrover/ES6-boilerplate

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 5, 2018

Looking at the standard itself, it doesn't support importing JSON directly. However, this may be a decent workaround until we figure out a better way of supporting ESM directly.

@eranshmil
Copy link
Contributor Author

@eranshmil eranshmil commented Nov 5, 2018

This is the quickest patch, the longer one is to create another directory with module.exports js files.
I guess I can make a patch directly in the grunt-videojs-languages package.
This is also not the best approach, but it's also won't take a lot of time.

What do you think?

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 5, 2018

Yeah, I think this is fine.

@gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 5, 2018

Going to merge even though the tests are failing as it's unrelated and may not be an issue in master. We're still investigating why the tests are failing.

@gkatsev gkatsev merged commit eb5de19 into videojs:master Nov 5, 2018
1 of 2 checks passed
@welcome
Copy link

@welcome welcome bot commented Nov 5, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@eranshmil eranshmil deleted the feat/es6-import-language branch Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants