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

Migrate demo from CRA to Vite #942

Merged
merged 6 commits into from
Jan 31, 2022
Merged

Migrate demo from CRA to Vite #942

merged 6 commits into from
Jan 31, 2022

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Jan 25, 2022

Tested on FF 60 (i.e. the "browser not supported" error shows up as expected).


EDIT - Summary of changes:

  • The demo is now based on Vite instead of CRA and no longer requires a config-override.js file to work.
  • Environment variables REACT_APP_DIST and STORYBOOK_DIST have been removed, as testing distributed packages this way is not sufficiently reliable and not worth the complexity it adds to the monorepo. Distributed packages can be tested by publishing beta versions. The four Codesandboxes (Add CRA and Vite sandboxes and fix @h5web/lib stylesheet export #938) are available to test the published packages reliably inside both CRA and Vite apps before publishing official releases.
  • The process of building the packages' styles is now more robust and a lot clearer. It is performed after the JS build and is based on a separate Vite config (vite.styles.config.js) and a separate entrypoint (src/styles.ts).
  • Consumers of @h5web/app now need to import a single stylesheet instead of two.
  • Building the packages, their styles and their type declarations, can now be done by running a single script: pnpm packages (no need to run pnpm packages:dts afterwards, as this is now done automatically).

@axelboc axelboc force-pushed the vite-demo-2 branch 6 times, most recently from 2248e5c to 6a3a034 Compare January 26, 2022 09:28
.gitignore Show resolved Hide resolved
apps/demo/index.html Show resolved Hide resolved
apps/demo/index.html Show resolved Hide resolved
apps/demo/src/H5GroveApp.tsx Show resolved Hide resolved
apps/demo/src/index.tsx Show resolved Hide resolved
apps/demo/src/index.tsx Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/app/src/explorer/Explorer.module.css Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
@axelboc axelboc marked this pull request as ready for review January 26, 2022 09:33
apps/demo/src/H5GroveApp.tsx Show resolved Hide resolved
apps/demo/src/index.tsx Outdated Show resolved Hide resolved
.github/workflows/approve-snapshots.yml Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
apps/demo/.env Show resolved Hide resolved
apps/demo/.env Show resolved Hide resolved
apps/demo/package.json Show resolved Hide resolved
apps/demo/src/styles.css Show resolved Hide resolved
apps/storybook/.storybook/main.js Outdated Show resolved Hide resolved
apps/storybook/.storybook/preview.js Show resolved Hide resolved
apps/storybook/package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
packages/app/src/App.tsx Show resolved Hide resolved
packages/lib/src/styles.css Show resolved Hide resolved
@axelboc
Copy link
Contributor Author

axelboc commented Jan 28, 2022

Alright, so as mentioned in the chat, removing the global styles imports from the index.ts files of the packages means that the packages' output stylesheets dist/style.css no longer include said global styles...

Instead of trying to find a sub-optimal solution that works for both the dev/build of the demo/Storybook and the build of the packages, I decided to take care of the problem on its own.

In the process, I managed to solve the problem of having to import two stylesheets when consuming the @h5web/app package. Now consumers just have to import @h5web/app/dist/styles.css (note the s). Also worth noting that I now run the build:dts script from the build script, so I've removed the packages:dts script from the root package.json.

I've explained how the packages are now build in details in the CONTRIBUTING guide. Hopefully it is clear. Relevant files (inside each package) are: package.json, vite.styles.config.js and src/styles.ts, README.md. Relevant commit: 71ae427

@axelboc axelboc marked this pull request as ready for review January 28, 2022 15:38
Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

👏

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
packages/app/src/styles.ts Outdated Show resolved Hide resolved
@axelboc axelboc force-pushed the vite-demo-2 branch 2 times, most recently from 973dd4f to 7700923 Compare January 31, 2022 09:37
@axelboc axelboc merged commit 2fd5f61 into main Jan 31, 2022
@axelboc axelboc deleted the vite-demo-2 branch January 31, 2022 09:58
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.

None yet

2 participants