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

Fix circular dependency and update final-form #1

Closed
wants to merge 3 commits into from

Conversation

ChrisOgden
Copy link

  • Upgrade final-form to v4.20.1 to remove controversial dependency
  • Change index.js for circular dependency issues when using inside another project

TylerRick and others added 3 commits October 14, 2020 22:52
Add some examples

API changes:
- Replace let:input let:meta with single let:field
- useFieldArray returns separate fields/meta stores if you want to destructure directly
- Change useFieldArray to return [form, state]
- Rename:
  - useForm -> getForm
  - createForm/createFormContext -> useForm
- useField: pass type through to input.type
  - Remove type from FormGroup/Input and get from input.type instead

Small fixes:
- Need <button type="button" so it doesn't default to submit
- Don't need to pass initialValues to form.reset()

Add missing features from react-final-form:
- Add/respect whenKeepDirtyOnReinitializeChanges prop
- Fix to make it work for checkboxes: actually use getValue from react-final-form
- Change Field to use <Input> by default to provide better parity with react-final-form
  - Add Input component
  - Update FormGroup, ErrorMessages to pass field, and to use Input by default (fallback for slot)
- ... still more to go
- Make component="select" work
- Make component="textarea" work

Svelte-specific:
- Forward input event/value: You can use on:input or bind:value on any of: Field, FormGroup, and Input
@ChrisOgden
Copy link
Author

These changes were made prior to your latest code. I can either work through the conflicts or let you. I haven't had a chance to look at your changes in depth yet.

@TylerRick
Copy link
Owner

Hey, Chris, thanks for this PR!

Yes, sorry, my dev branch is still in constant flux (and subject to a lot of commit amending/rewriting) right now, so I'm not surprised there were a lot of merge conflicts, even for such a simple change.

You may want to hold off on any PRs for now, other than ones that add new examples ✨ 😀 ...


Circular dependencies

I had seen these warnings from rollup:

(!) Circular dependencies
src/index.js -> src/useForm.js -> src/index.js
src/index.js -> src/useForm.js -> src/useFormState.js -> src/index.js
src/index.js -> src/Field.svelte -> src/index.js
src/index.js -> src/Input.svelte -> src/index.js
src/index.js -> src/arrays/index.js -> src/arrays/useFieldArray.js -> src/index.js

but since it seemed to build and work fine as-is, and since I don't see any circular dependencies present in the dist/index.mjs module that consumers will actually be importing from, I don't think this is an actual problem at the moment — more like just a warning of a possible problem.

As such, I've just been treating those warnings as something to ignore for now. (See also these examples of other people knowingly having circular dependencies and choosing to ignore the warnings.) You're probably right that we should address them at some point though. As long as we can do so cleanly...

But I'd much rather have something concise like this:

  import { useField, Input, useForwardEvent } from '.'

than have it verbosely list out each individual "actual" source file path like this (which is longer to type, less maintainable, creates more duplication, causes more churn, and makes for more relative paths that need to be updated if any files get moved around):

  import { default as Input } from './Input.svelte'
  import { default as useField } from './useField'
  import { useForwardEvent } from './useForwardEvent.js'

So I'd prefer to find a different solution to those circular dependencies.

I'm thinking... instead of importing directly from our main index.js module, we just import from a internal module instead. This pattern was described pretty well here and here is an example of such a refactoring. What do you think?

Dependency upgrade

As far as the dependency upgrade, sounds great!

I've cherry picked that commit and will push it up soon.

@TylerRick TylerRick closed this Oct 22, 2020
@TylerRick TylerRick deleted the branch TylerRick:dev October 22, 2020 02:17
@ChrisOgden
Copy link
Author

Looking at this again I agree the circulate dependency items can be ignored for now, it stuck out to me when doing development and I think it is only occurring because I am using yarn:link to work with this repo currently. I believe if it was using the build folder it wouldn't have this problem.

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