adds dynamic loading to video-test #698
Conversation
joelhooks
commented
Jun 16, 2021
joelhooks
commented
|
This pull request is being automatically deployed with Vercel (learn more). egghead-io-nextjs – ./🔍 Inspect: https://vercel.com/eggheadio/egghead-io-nextjs/8r2cCPZxBg8D4ZVUwR3qxpzACN9M egghead-next-storybook – ./🔍 Inspect: https://vercel.com/eggheadio/egghead-next-storybook/4aT33cHU91dxKwmWVDnWXCn6cMSA |
| @@ -126,8 +132,11 @@ | |||
| "tailwindcss": "^2.1.4", | |||
| "tincanjs": "^0.50.0", | |||
| "ts-toolbelt": "^9.6.0", | |||
| "unist-util-visit": "^2.0.3", | |||
There was a problem hiding this comment.
this is an older version of this library
syntax-tree/unist-util-visit#23
apparently the entire unified ecosystem is making a principled stance on using ESM modules which is incompatible with Next.js (and most of the rest of the world) at this time. This causes a massive headache and debugging nightmare to implement, but it's a principled stance and open source so you can only complain about it so much I guess.
Kind of sucks when you just want to make stuff, particularly since it'd be relatively simple to support commonjs syntax for now.
This issue, in fact, has caused mdx to be forked 
I don't like it.
There was a problem hiding this comment.
Maintainer of unified and mdx here, sorry you ran into a spot of trouble.
This characterization is unfair on a few fronts:
- It is not a libraries fault if a build tool does not support standard JavaScript syntax.
- Next has been working to add ESM support, and merged add support for esm externals vercel/next.js#27069 to address this specifically
- MDX was forked over a disagreement on how to handle presentation context and runtime Remove runtime renderer, still support
MDXProvidermdx-js/mdx#1425 and An alternative “shortcode” (mdxTypealternative) proposal mdx-js/mdx#1385, it has nothing to do with ESM - Unified had experimented with adding a CJS fallback to support older versions of CRA and Next, but due to some quirks of webpack 4, it actually made things harder for adopters than pure ESM 🤷♂️
If you have questions on ESM or MDX in general, feel free to reach out to the MDX community https://github.com/mdx-js/mdx/discussions and/or unified community https://github.com/unifiedjs/unified/discussions
There was a problem hiding this comment.
Thanks for this detailed list of corrections.
| @@ -98,7 +98,7 @@ const NoteCue: React.FC<any> = ({ | |||
| const visible = | |||
| player.activeMetadataTrackCues.includes(cue) && !player.seeking | |||
| const startPosition = `${(cue.startTime / duration) * 100}%` | |||
| const note = JSON.parse(cue.text) | |||
| const note = cue.text | |||
There was a problem hiding this comment.
not using JSON, which might be something we test for in the future. We can just not needed for the current approach.
|
|
||
| export default loadGithubNotes | ||
|
|
||
| export async function loadNotesFromUrl(url: string) { |
There was a problem hiding this comment.
this loads mdx from a url and parses it looking for timestamp components which creates JS objects that are used to make a webvtt.
it's currently very fragile! works though
