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 Dashboard components to React components library #24

Merged
merged 3 commits into from
Feb 16, 2022

Conversation

pkosiec
Copy link
Member

@pkosiec pkosiec commented Feb 10, 2022

Description

Changes proposed in this pull request:

  • Migrate Dashboard components to React components library
  • Fix build setup for components

Unfortunately I ran out of time to:

  • Prepare Stylebook examples for all components
  • Refactor components to use Styled Components
  • I also wasn't able to fix the race condition in development setup build.

I will create a follow-up issue for that.

Relations

Needs #23 to be merged first

Testing

Follow the Readme to start app locally.
Also, see the sample repo with external usage of the components: https://github.com/pkosiec/dashboard-components-example

Issues

Among other things:

  • crypto package included in components build

The problem was:

 ERROR in ../react-components/dist/esm/index.js 7:0-23
@capactio/dashboard-ui: Module not found: Error: Can't resolve 'crypto' in '/Users/pkosiec/repositories/dashboard/react-components/dist/esm'
@capactio/dashboard-ui: BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
@capactio/dashboard-ui: This is no longer the case. Verify if you need this module and configure a polyfill for it.
@capactio/dashboard-ui: If you want to include a polyfill, you need to:
@capactio/dashboard-ui:         - add a fallback 'resolve.fallback: { "crypto": require.resolve("crypto-browserify") }'
@capactio/dashboard-ui:         - install 'crypto-browserify'
@capactio/dashboard-ui: If you don't want to include a polyfill, you can use an empty module like this:
@capactio/dashboard-ui:         resolve.fallback: { "crypto": false }

Initially I checked facebook/create-react-app#11756 but it didn't seem right to have this error now, if previously we haven't had such issue.
Turned out that the react-jsonschema-form includes nanoid as dependency, which needed another customization in Rollup config to use the browser environment: https://github.com/ai/nanoid#rollup

  • bundling React inside components library: solution is to use Lerna hoist and a proper Rollup configuration
  • circular dependencies
@capactio/react-components: (!) Circular dependencies
@capactio/react-components: ../node_modules/@rjsf/core/dist/es/utils.js -> ../node_modules/@rjsf/core/dist/es/components/fields/index.js -> ../node_modules/@rjsf/core/dist/es/components/fields/ArrayField.js -> ../node_modules/@rjsf/core/dist/es/utils.js
@capactio/react-components: ../node_modules/@rjsf/core/dist/es/utils.js -> ../node_modules/@rjsf/core/dist/es/components/fields/index.js -> ../node_modules/@rjsf/core/dist/es/components/fields/BooleanField.js -> ../node_modules/@rjsf/core/dist/es/utils.js
@capactio/react-components: ../node_modules/@rjsf/core/dist/es/utils.js -> ../node_modules/@rjsf/core/dist/es/components/fields/index.js -> ../node_modules/@rjsf/core/dist/es/components/fields/MultiSchemaField.js -> ../node_modules/@rjsf/core/dist/es/utils.js
@capactio/react-components: ...and 26 more

This is just a warning. It is a known issue: rjsf-team/react-jsonschema-form#2341 - cannot do anything about it

Related issue(s)

Resolves #21

@pkosiec pkosiec added enhancement New feature or request WIP Work in progress area/dashboard Relates to Dashboard labels Feb 10, 2022
@pkosiec pkosiec marked this pull request as ready for review February 12, 2022 00:41
@pkosiec pkosiec removed the WIP Work in progress label Feb 12, 2022
Copy link

@mkuziemko mkuziemko left a comment

Choose a reason for hiding this comment

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

I verified it locally and it works ok 👍 On my end, I noticed also the ,,race" and I needed to restart the app to work but it is not a big deal. :)

README.md Outdated
Comment on lines 51 to 52
This command watches for changes to Dashboard of any related packages to build
them and live reload the page.

Choose a reason for hiding this comment

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

Suggested change
This command watches for changes to Dashboard of any related packages to build
them and live reload the page.
This command watches for changes to Dashboard of any related packages to build them and live reload the page.

react-components/src/wizard/Wizard.tsx Outdated Show resolved Hide resolved
@pkosiec
Copy link
Member Author

pkosiec commented Feb 16, 2022

On my end, I noticed also the ,,race" and I needed to restart the app to work but it is not a big deal. :)

Fortunately, you don't need to restart build - you can just e.g. add newline to any of the react component file, and it will trigger Dashboard rebuild with latest react components. So the race sometimes forces you to make double changes to see the initial change on Dashboard.

@pkosiec pkosiec merged commit a49f60a into capactio:main Feb 16, 2022
@pkosiec pkosiec deleted the migrate-components branch February 16, 2022 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dashboard Relates to Dashboard enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare setup for publishing Capact Dashboard React components
3 participants