Skip to content

chore: cleanup #281

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

Merged
merged 23 commits into from
Apr 8, 2024
Merged

chore: cleanup #281

merged 23 commits into from
Apr 8, 2024

Conversation

anuragxxd
Copy link
Member

@anuragxxd anuragxxd commented Mar 23, 2024

Description

Fixes #147 #139 #269 #268 #244 #243 #238

Checklist

  • My code follows the contributing guidelines of this project.
  • I am aware that all my commits will be squashed into a single commit, using the PR title as the commit message.
  • I have performed a self-review of my own code.
  • I have commented my code in hard-to-understand areas.
  • I have updated the user-facing documentation to describe any new or changed behavior.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have not reduced the existing code coverage.

Comments

Copy link

changeset-bot bot commented Mar 23, 2024

⚠️ No Changeset found

Latest commit: eb7e133

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Mar 23, 2024

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

Name Status Preview Comments Updated (UTC)
cloud-components ❌ Failed (Inspect) Apr 8, 2024 9:34am

@anuragxxd
Copy link
Member Author

Hey @SalihuDickson @JaeAeich, I'd appreciate it if you could take a look at this PR. I've cleaned things up by replacing older packages with their newer counterparts. Since this is a large PR, here are the key areas to focus on:

  • @SalihuDickson I removed the deprecated dependency from the build script that was causing errors. I also updated the make-react script's component path to address another error. To verify these changes, it would be helpful if you could run the script locally.
  • @JaeAeich I've made some changes to WES & TES as well. If you have time, it would be great if you could run them locally to ensure they're working as expected.
  • @JaeAeich We're removing the docs entirely in favor of feat(docs): Nextra docs #262 so there's no need to migrate them. Could you consider updating your PR to use the latest packages?
  • commit-lint & commit-generator: To streamline our squash & merge strategy, I've removed these from the recommit.

I'm also considering moving TRS & TRS-filer to a legacy folder to deprecate them before migrating them to the lit & design package implementation. What are your thoughts on this approach?

For peace of mind, it would be great if you both could try running the PR locally to check any functionalities it might affect. While I've tested everything myself, more reviews are always valuable. thanks!

Copy link
Contributor

@JaeAeich JaeAeich left a comment

Choose a reason for hiding this comment

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

@git-anurag-hub

  • Please change the license from MIT to Apache 2.0 for individual packages as well (ref).
  • Please configure the dev server ports to be different they are running on 8000 currently.
  • Building the package is really unstable and breaks quite often, maybe the clean up before build should fix it.
  • dev fails if the user hasn't run build, maybe we need to create a custom script that build the dependency of the packages (we can provide our dependecy list or just infer all the deps that have "*"), if they are already built runs the dev else builds the deps and then runs.
  • husky if we are keeping that, then lets add lint-staged, I mean otherwise I don't see a point of running the test each time I have to commit, yeah would have sense to lint some files (staged).
  • prettier please can we also configure prettier in this PR itself, only formatting md, mdx, html, css, JSON and other non code files only though. And since this can be added to husky, this might be useful.

@JaeAeich
Copy link
Contributor

I'm also considering moving TRS & TRS-filer to a legacy folder to deprecate them before migrating them to the lit & design package implementation. What are your thoughts on this approach?

Let's not move them to a different package for the sake of consistency. Instead, we could remove and/or comment out their scripts so that they don't build or run their tests in CI.

@JaeAeich
Copy link
Contributor

Also please run npm i and update the lock file. Apart from that, take a look into this #252 either take reference or configure the package acc you in this PR itself.

PS: If so please also make the changes in template files.

@SalihuDickson
Copy link
Contributor

SalihuDickson commented Mar 25, 2024

@JaeAeich, I'm not sure how much time @git-anurag-hub has on his hands, and this PR looks like it still needs quite a bit of work and testing before it is ready to be merged. To avoid a lot of back and forth, especially considering the size of the PR, I think we should just make PRs to this fixing the issues we notice.

@git-anurag-hub, please correct me if you would prefer we just request changes. I'm just trying to get this out as soon as possible and it seems it would be easier and quicker for you to look through and approve our PRs instead of having to go and make changes and request reviews to start the process all over again.

@anuragxxd
Copy link
Member Author

@git-anurag-hub

  • Please change the license from MIT to Apache 2.0 for individual packages as well (ref).
  • Please configure the dev server ports to be different they are running on 8000 currently.
  • Building the package is really unstable and breaks quite often, maybe the clean up before build should fix it.
  • dev fails if the user hasn't run build, maybe we need to create a custom script that build the dependency of the packages (we can provide our dependecy list or just infer all the deps that have "*"), if they are already built runs the dev else builds the deps and then runs.
  • husky if we are keeping that, then lets add lint-staged, I mean otherwise I don't see a point of running the test each time I have to commit, yeah would have sense to lint some files (staged).
  • prettier please can we also configure prettier in this PR itself, only formatting md, mdx, html, css, JSON and other non code files only though. And since this can be added to husky, this might be useful.

@JaeAeich

  • Sounds good! I've updated the LICENSE files in all individual packages to Apache 2.0.
  • Seems like all are pointed to the different ports, but still have reconfigured and arranged them more sensibly.
  • I guess (atleast I hope) this will not be the problem after the refactor is completed. In the meantime, we'll continue investigating and resolving any build-related problems.
  • I think the better way of this will be for the dev's to start the dev env for the dependencies (not the build) so that they they can leverage features like HMR for better debugging. Meanwhile will explore automating this process by configuring turbo.json, similar to the build command.
  • Good idea, this is the reason why we have removed most of the pre-commit hooks now to reduce the overhead from the developers but I guess it is not the best idea to use the Github runner minutes for the test & lint scripts.
  • We can have issue for this!

@anuragxxd
Copy link
Member Author

@JaeAeich, I'm not sure how much time @git-anurag-hub has on his hands, and this PR looks like it still needs quite a bit of work and testing before it is ready to be merged. To avoid a lot of back and forth, especially considering the size of the PR, I think we should just make PRs to this fixing the issues we notice.

@git-anurag-hub, please correct me if you would prefer we just request changes. I'm just trying to get this out as soon as possible and it seems it would be easier and quicker for you to look through and approve our PRs instead of having to go and make changes and request reviews to start the process all over again.

Sure @SalihuDickson, while I have addressed most of the issues referenced here. Are there any other issues you think which are left here? If so please let me know. I have tested most of the functionality affected by this PR but want you & @JaeAeich to take a look at the parts where you have worked. Saying this please go ahead & fix the issues that you might think can take less time solving parallely.

@JaeAeich
Copy link
Contributor

@anuragxxd for some reason, I can't build the packages propoerly, maybe we need to have discussion on this. Is it working fine on your end. I did get it running but that seems to be an on off thing, its not consistent. I can't be sure the packages will definetely build everytime.

@JaeAeich
Copy link
Contributor

@anuragxxd for some reason, I can't build the packages propoerly, maybe we need to have discussion on this. Is it working fine on your end. I did get it running but that seems to be an on off thing, its not consistent. I can't be sure the packages will definetely build everytime.

Please merge the fix and some more cleanup from #285.

JaeAeich
JaeAeich previously approved these changes Mar 30, 2024
Copy link
Contributor

@JaeAeich JaeAeich left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Small PRs from now on please 😮‍💨 🤣 !

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: salihuDickson <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@JaeAeich
Copy link
Contributor

JaeAeich commented Apr 6, 2024

LGTM, Everything seems to work on my machine, please resolve the above as you see fit and lets merge 🚀

Signed-off-by: Anurag Gupta <[email protected]>
@anuragxxd
Copy link
Member Author

@SalihuDickson @JaeAeich alright, gonna go ahead and merge this if it all looks good to you. have tested the changes on my machine too seems to work fine.

Copy link
Contributor

@SalihuDickson SalihuDickson left a comment

Choose a reason for hiding this comment

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

lgtm

@anuragxxd anuragxxd merged commit 9c88bd4 into main Apr 8, 2024
@anuragxxd anuragxxd deleted the cleanup branch April 8, 2024 23:18
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.

feat(wes): migrate wes to lit
3 participants