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

Add missing react next demo packages #10495

Closed
wants to merge 1 commit into from

Conversation

leriel
Copy link

@leriel leriel commented Sep 1, 2023

Context

Please do not merge this pull request. I am creating it only to better understand packages spread across the repo. As i am preparing to create PR that's going to overwrite much of this demo

What i did:

  1. Clone repo
  2. cd examples/next/docs/react/demo/
  3. npm install
  4. npm run start
  5. npm run build
  6. npm run test

Steps 4 and 5 failed with

[eslint] Failed to load config "react-app" to extend from.

Because of failed build i could not run tests. When i added eslint-config-react-app i could start, build, and run tests.
As i understand this, eslint-config-react-app package is currently missing from this demo. However, given how it's present in every demo package-lock.json, i understand i may be missing a step

npm --version
9.8.0
node --version
v20.5.0

I understand that "react-scripts" (https://github.com/handsontable/handsontable/blob/develop/examples/next/docs/react/demo/package.json#L25) have eslint-config-react-app dependency. For some reason, latter does not install.

Thanks

How has this been tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

Affected project(s):

  • handsontable
  • @handsontable/angular
  • @handsontable/react
  • @handsontable/vue
  • @handsontable/vue3

Checklist:

@evanSe
Copy link
Member

evanSe commented Oct 2, 2023

Hey @leriel,

I just wanted to say thanks for the work you have done so far; it has actually pushed me in the right direction to find a solution for the issues with StackBlitz!

As I began investigating, I noticed two things:

  1. StackBlitz had a bug on their side, which they have since resolved.
  2. StackBlitz is not very straightforward in its handling of the package.json file. It essentially creates its own version and tends to ignore the majority of what is in the GitHub repository. For instance, it removes react-app-rewired and discards it. (Ideally, in the future, Handsontable examples should use Vite or a production-grade framework as recommended in the React documentation.)

Hopefully, in the near future, we can merge the following pull request, which addresses the issue for versions ^13.0.0: #10523. Fixing prior versions can also be done but is quite tedious.

For now, I will close this pull request. Feel free to reopen or reach out to me if I missed anything. Thanks again!

@evanSe evanSe closed this Oct 2, 2023
@leriel
Copy link
Author

leriel commented Oct 3, 2023

Hi @evanSe, nice to meet you! :)

Much like you, i discovered vite based projects are treated differently by stackblitz - they are being assigned webcontainer environment. Reason why i haven't closed this PR and opened next is, because unfortunately stackblitz /github endpoint hangs up on cloning some of my branches :/

This is react demo with react-app-rewired replaced with vite. Back when it still did clone, it worked xD Currently it hangs on cloning

It has:

  • all tests retained and all tests passing
  • changed from ts to js (for simplicity, no issue having vite/ts though)

This is more simple example that still clones to this day and demonstrates working hot+vite: https://stackblitz.com/github/leriel/stackblitz-vite-test/tree/handsontable?file=package.json

I later discovered that plain CRA does work on stackblitz. Here is stackblitz link to my branch, which has just rewired replaced with CRA, but again it hangs cloning. Branch here

Let me know if opening PR with swap to either CRA or vite helps you. Until they can be actually cloned by stackblitz, it doesn't seem like it would help though. I can change hot+vite to ts as well

@leriel
Copy link
Author

leriel commented Oct 3, 2023

As for Angular demo, there is actually a way to make it work with vite. I'm not sure how valuable would that be for you (i would suspect angular community is 99,9% ng based), but i'm happy to hack up angular+vite demo :)

@evanSe
Copy link
Member

evanSe commented Oct 6, 2023

@leriel, thanks for the detailed information! 😄 In the coming days, I will investigate further and let you know if I manage to resolve any of the cloning issues. So far, StackBlitz and the custom things they are doing can be super confusing.

@leriel
Copy link
Author

leriel commented Oct 13, 2023

@evanSe sure, glad to help! If you want me to try things with those branches i mentioned, just ping me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants