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

[BUG] filterDOMProps and uniforms-material package #815

Closed
simplecommerce opened this issue Oct 18, 2020 · 15 comments · Fixed by #830
Closed

[BUG] filterDOMProps and uniforms-material package #815

simplecommerce opened this issue Oct 18, 2020 · 15 comments · Fixed by #830
Assignees
Labels
Type: Bug Bug reports and their fixes
Milestone

Comments

@simplecommerce
Copy link
Contributor

simplecommerce commented Oct 18, 2020

Hi,

Did something change in the filterDOMProps and uniforms-material package in 3.0.0-rc.5?

I have this example:

console.log(filterDOMProps({
      className: "SCComponent-submitBtn-75",
      color: "primary",
      variant: "contained",
      fullWidth: true
    }))

This used to return the whole object, but now in the latest version it only returns className and color, variant and fullWidth is missing.

You can see the example below, updated both the uniforms and uniforms-material package to the rc.5 and it causes this issue.

https://codesandbox.io/s/currying-snow-jk55s?file=/App.tsx

Any ideas?

@radekmie radekmie added this to Needs triage in Open Source (migrated) via automation Oct 23, 2020
@radekmie radekmie self-assigned this Oct 23, 2020
@radekmie radekmie added the Type: Question Questions and other discussions label Oct 23, 2020
@radekmie radekmie moved this from Needs triage to In progress in Open Source (migrated) Oct 24, 2020
@radekmie
Copy link
Contributor

Hi @simplecommerce. Yes, it was changed in #803, to remove wrapField._filterDOMProps. I can see that it's not documented in the changelog either. Is this a problem for your project or you're just asking if it changed in the package itself?

@simplecommerce
Copy link
Contributor Author

@radekmie Hi, thanks for the response, yes it affects my project because fullWidth and variant are available props that work on material-ui components, and in one of my scenario, I created a custom component (button) and I am using filterDOMProps(props) to remove all uniforms props, I wasn't expecting it to remove both fullWidth and variant also.

@radekmie
Copy link
Contributor

OK, a workaround, for now, is to set these two props manually. I'll think about whether we should revert this change or not. Also, I'd like to hear your opinion on that.

@simplecommerce
Copy link
Contributor Author

@radekmie Not to sound too stupid, but I am not sure I understand the reason for the change that impacts the filterDOMProps function. I think in my case, if I want to prevent having other issues due to similar changes that could arise because of improvements, I would have to create my own filterDOMProps function and remove the original uniforms props.

In my use case, since I am creating custom components, I only want to filter the original uniforms props and allow every other props to be passed to the underlying component.

I was thinking, if there would be a way to pass a prop or an argument to the filterDOMProps to ignore the removal of the material-ui package props?

Or something like that.

If not, its no problem, there is always a work around, I just wanted to be sure it was intentional.

@radekmie
Copy link
Contributor

So the reason for that is that the wrapField helper understand some common props, like variant, and otherwise we'd need to remove them in every field (like we did, using internal wrapField._filterDOMProps).

@simplecommerce
Copy link
Contributor Author

@radekmie Ok I see, is it possible for filterDOMProps to work on its own removing uniforms props, and the wrapField extend filterDOMProps without altering its behavior if used on its own, something like that?

@radekmie
Copy link
Contributor

No, at least not right now, as filterDOMProps is a shared function. We've used a separate function for that before.

@asibilia
Copy link
Contributor

@radekmie does this mean we're not able to pass variant props to Textfields? I'm currently trying to pass variant="outlined" to AutoField but doing so only applies the outlined styling to select fields.

@radekmie
Copy link
Contributor

@asibilia Indeed, it won't work now. We've also found a few other places where it is a problem. We have to revert this change.

@radekmie radekmie added Type: Bug Bug reports and their fixes and removed Type: Question Questions and other discussions labels Nov 17, 2020
@radekmie radekmie moved this from In progress to To do in Open Source (migrated) Nov 17, 2020
@radekmie radekmie added this to the v3.0 milestone Nov 17, 2020
@radekmie radekmie moved this from To do to In progress in Open Source (migrated) Nov 22, 2020
@radekmie
Copy link
Contributor

@simplecommerce, @asibilia - would any of you be able to check, whether #830 solves your problems?

@simplecommerce
Copy link
Contributor Author

@radekmie don't want to sound dumb but how would I test it?

@radekmie
Copy link
Contributor

@simplecommerce You'd have to clone the repo locally, set it up (npm ci should be enough) and use local version of uniforms instead of the npm one (using file: pseudo-version; npm i path-to-root/packages/uniforms-material).

@simplecommerce
Copy link
Contributor Author

simplecommerce commented Nov 25, 2020

@radekmie I think I am missing a step, I tried cloning the repo and the filter-props branch and then ran npm install in the root folder, after that I went to my project and ran npm install /path-to-root/packages/uniforms-material.

Seems to be ok then, I tried running my app and got this error.

Error: Cannot find module '/root/v3/backend/backend/node_modules/uniforms-material/es5/index.js'. Please verify that the package.json has a valid "main" entry

I think I am supposed to npm run build in the repo but it fails saying it can't find uniforms module.

Am I missing something?

Update: I ran npm run install instead it seems to be doing something, i am waiting for it to finish.

Ok so npm run install worked, after I installed it in my app, but now when I run it I get this error.

Uncaught Invariant Violation: useForm must be used within a form.

I tried uninstalling the package and installing the one from the repo, and it works again.

So I am wondering if anything else changed?

Update 2: Ok so I had to also npm install uniforms from the same local package. Upon first glance, it seems to work fine now with the revert, I commented my custom filterProps and the package one did its job, so it seems good to me.

If @asibilia wants to confirm also.

@radekmie
Copy link
Contributor

@simplecommerce Thanks! The first problem seems a bit odd as npm ci (or npm i and npm install) should run npm run build anyway. Out of curiosity: what is your npm version?

@simplecommerce
Copy link
Contributor Author

@radekmie yeah i had to specifically type npm run install to make it build and not just npm install and npm run build.

my npm version is 6.13.4

Open Source (migrated) automation moved this from In progress to Closed Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug reports and their fixes
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants