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

Enable the example projects to use locally linked versions of the Vime packages #227

Merged
merged 15 commits into from
Jul 14, 2021

Conversation

rossng
Copy link
Contributor

@rossng rossng commented Jul 13, 2021

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Currently, it is not possible to use npm link or Lerna to use the local version of the @vime/* packages. There were a number of issues preventing this, all of which should be fixed by this PR.

The main motivation for this change is to make it possible to quickly test local changes to @vime/core without any ceremony.

Issue Number: N/A

What is the new behavior?

  • examples/* has been added to the Lerna workspaces, making it possible to bootstrap the @vime/* packages in these projects.
  • npm run build has been changed to run only on the public packages (i.e. no private: true). This preserves the existing behaviour that examples will not be built during CI.
  • A new npm run bootstrap:all command is introduced to allow bootstrapping the examples if desired.
  • Documentation added for this behaviour
  • A number of fixes required for the example projects:
    • @vime/core: copy the icons/ folder structure correctly, so icons load when NODE_ENV === development.
    • @vime/react-example: resolve react and react-dom correctly when using linked packages
    • @vime/rollup-example: update the CSS extension to make CSS generation work
    • @vime/stencil-example: fix icons not being found in dev mode
    • @vime/webpack-example: ensure Webpack always loads the extracted CSS
    • @vime/angular-example: preserve symlinks in Angular config

Does this introduce a breaking change?

  • Yes
  • No

Other information

The local icons are used at development time, but were incorrectly copied without preserving the directory structure by cpy-cli. This commit moves that responsibility to a Stencil copy task.
Previously this would result in duplicate copies of React.
This means that they can be used with locally linked versions of the @vime packages if desired. By default, the build command will only run on the public packages.
@vercel
Copy link

vercel bot commented Jul 13, 2021

@rossng is attempting to deploy a commit to the Vidstack Team on Vercel.

A member of the Team first needs to authorize it.

@mihar-22
Copy link
Contributor

Looks awesome to me. Not sure if this is related but getting some TS errors in CI? Seems unrelated to your changes though. Maybe we can patch in another PR. I also invited you to join the Vime organization so you can have more access. Let me know what you need.

Thank you for the PR!

image

@mihar-22 mihar-22 self-requested a review July 13, 2021 14:51
Copy link
Contributor

@mihar-22 mihar-22 left a comment

Choose a reason for hiding this comment

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

LGTM

@mihar-22
Copy link
Contributor

Feel free to merge once you've accepted invite. Just ping me for reviews on larger changes 😄

@rossng
Copy link
Contributor Author

rossng commented Jul 13, 2021

Just added another change to switch the Angular packages to the v11-lts tag - should avoid incompatible peer deps errors when using NPM 7.

@rossng
Copy link
Contributor Author

rossng commented Jul 13, 2021

Having a look at the build error now.

@rossng
Copy link
Contributor Author

rossng commented Jul 13, 2021

After some fiddling CI seems to be working again 🙂.

I don't have write permissions on the repo, but probably best if you look over the last few commits anyway @mihar-22!

Probably no need to do a release after this merge, there will be some actual end-user facing stuff coming up soon.

@mihar-22
Copy link
Contributor

Awesome. You have write access now. The changes look good to me. It'd be nice if we didn't have to duplicate .prettierignore and instead extend .gitignore somehow? Either way fine with me.

@vercel
Copy link

vercel bot commented Jul 14, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vidstack/vime/3KsjDDPoeM5p6PgDgc9d1ETSaL3h
✅ Preview: https://vime-git-fork-rossng-allow-example-linking-vidstack.vercel.app

@vercel vercel bot temporarily deployed to Preview July 14, 2021 02:00 Inactive
@rossng
Copy link
Contributor Author

rossng commented Jul 14, 2021

I thought the same, but seems to be unsolved by Prettier so far: prettier/prettier#8506

@rossng rossng merged commit fac1f7c into vime-js:main Jul 14, 2021
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.

2 participants