Skip to content

Upgrade to CRA 5, rename cjs to .cjs.js#225

Merged
gkatsev merged 2 commits into
muxinc:mainfrom
gkatsev:chore/upgrade-cra
May 19, 2022
Merged

Upgrade to CRA 5, rename cjs to .cjs.js#225
gkatsev merged 2 commits into
muxinc:mainfrom
gkatsev:chore/upgrade-cra

Conversation

@gkatsev
Copy link
Copy Markdown
Contributor

@gkatsev gkatsev commented May 18, 2022

Supersedes #210, inspired by #224

@vercel
Copy link
Copy Markdown

vercel Bot commented May 18, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
elements-demo-create-react-app ✅ Ready (Inspect) Visit Preview May 19, 2022 at 5:40PM (UTC)
elements-demo-nextjs ✅ Ready (Inspect) Visit Preview May 19, 2022 at 5:40PM (UTC)
elements-demo-svelte-kit ✅ Ready (Inspect) Visit Preview May 19, 2022 at 5:40PM (UTC)
elements-demo-vanilla ✅ Ready (Inspect) Visit Preview May 19, 2022 at 5:40PM (UTC)
elements-demo-vue ✅ Ready (Inspect) Visit Preview May 19, 2022 at 5:40PM (UTC)

@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2022

Codecov Report

Merging #225 (2ab9243) into main (b340ee0) will decrease coverage by 0.47%.
The diff coverage is n/a.

❗ Current head 2ab9243 differs from pull request most recent head 8afa153. Consider uploading reports for the commit 8afa153 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
- Coverage   82.66%   82.19%   -0.48%     
==========================================
  Files          37       37              
  Lines        4004     4004              
  Branches      132      131       -1     
==========================================
- Hits         3310     3291      -19     
- Misses        688      704      +16     
- Partials        6        9       +3     
Impacted Files Coverage Δ
packages/mux-player/src/index.ts 75.11% <0.00%> (-2.01%) ⬇️
.../mux-player/src/media-theme-mux/media-theme-mux.ts 98.52% <0.00%> (-0.74%) ⬇️

Copy link
Copy Markdown
Contributor

@luwes luwes left a comment

Choose a reason for hiding this comment

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

🚢 !

Copy link
Copy Markdown
Contributor

@cjpillsbury cjpillsbury left a comment

Choose a reason for hiding this comment

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

blocker: has this been tested against CRA 4/older webpack? I noticed all cjs bundles were removed from the react packages, and I thought the assumption was cjs was expected for those cases.

@gkatsev
Copy link
Copy Markdown
Contributor Author

gkatsev commented May 19, 2022

@cjpillsbury it's because esm builds were added to the react packages. We can remove those, if we wanted.

Good call on trying it out with current CRA, let me try it out locally.

@gkatsev
Copy link
Copy Markdown
Contributor Author

gkatsev commented May 19, 2022

And I need to resolve the merge conflict again

@gkatsev
Copy link
Copy Markdown
Contributor Author

gkatsev commented May 19, 2022

CRA4 with the two changes: #227
Has the following error:

Can't import the named export 'StreamTypes' from non EcmaScript module (only default export is available)

@gkatsev gkatsev force-pushed the chore/upgrade-cra branch from 2ab9243 to a043498 Compare May 19, 2022 16:51
@gkatsev gkatsev changed the title Upgrade to CRA 5, rename cjs to .cjs.js, add esm builds to react packages Upgrade to CRA 5, rename cjs to .cjs.js May 19, 2022
@gkatsev
Copy link
Copy Markdown
Contributor Author

gkatsev commented May 19, 2022

I've removed the addition of the esm builds for the react packages because they seem to break CRA4.

@gkatsev
Copy link
Copy Markdown
Contributor Author

gkatsev commented May 19, 2022

The CRA build of vercel is now passing in #227: https://elements-demo-create-react-app-git-fork-gkatsev-cjs-798162-mux.vercel.app/

Copy link
Copy Markdown
Contributor

@cjpillsbury cjpillsbury left a comment

Choose a reason for hiding this comment

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

non-blocking nit: looks like I/you generated the updated CRA app w/npm instead of yarn so the docs changed. We should probably still recommend folks use yarn, but this is def lo pri.

Otherwise, looking 😎, tested across different apps.

cjpillsbury and others added 2 commits May 19, 2022 13:36
Some build tools don't understand cjs file extensions yet.
Notably, Create React App
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.

3 participants