-
Notifications
You must be signed in to change notification settings - Fork 2k
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
example: revive react native example #4164
example: revive react native example #4164
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to contribute!
Wow, this monorepo is a bit of a nightmare sadly 😞
It's not the easiest, but Uppy is a complex project.
eslint is screwed for some reason I still have to investigate, therefore I disabled it for now
I don't think we should disable it for this example. What exactly went wrong?
In fact, this code I'm pushing does work in the sense that the following application starts, but then I'm encountering some error in uppy's core, regarding the usage of the tus browser wrapper that uses standard browser features unavailable to react-native of course:
Unfortunately I have never worked with React Native nor the Uppy plugin for it. If uploaders don't work, we should take a look at that indeed.
Sorry for having paused this. Tomorrow I'll give it a go 😃 I'll start with the linter and follow to rewrite the entire project |
Linting is ok now. Having said this, I don't know if it's a smart choice to import @uppy/react within the react native example since I don't know if it's platform agnostic (that is, doesn't use any browser API). Any thoughts? @Murderlon |
Thanks for looking into this again.
This comes from the thumbnail generator, which indeed uses browser APIs. If no browser APIs are possible in react native, I'm unsure about the value of |
I don't completely disagree but:
So, I'd remove the uppy/react-native package as a whole, make uppy/react agnostic (do you think it's easily possible?), and then we'll be able to provide users a react-native-expo example that makes direct use of the expo document picker and so on, leaving it then to the users so that you would automatically also "support" react-native standalone projects. If we agree on all, I'd pause this pr (leaving it open) and open a new pr to remove browser apis dependency from uppy/react (or at least implement conditions so that if the code is not run in a browser, it can still be imported, which would also be beneficial for server side components or server side rendered applications I suspect) |
I have some doubts though since your @uppy/react took the same direction as @uppy/react-native. That is, providing UI components that "automagically" handle uploads. For this reason I think it's quite hard to make @uppy/react platform agnostic 😞 |
It's impossible to make |
then the easiest solution is to rollout a complete ad-hoc solution for react native based on what's available right now. |
Also to keep a cohesive experience, we could divide browser-tied features and non-browser features of the uppy/react package so as to replicate just the seconds (for now) in uppy/react-native's package. AFAIK, the only non-browser-based feature in uppy/react is the useUppy hook, since all the others are components to be run in web, right? |
rules: { | ||
'no-unused-vars': [ | ||
'error', | ||
{ | ||
'varsIgnorePattern': 'React', | ||
}, | ||
], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why the |
Maybe I didn't explain myself clearly in that message but never mind. |
Interesting, that shouldn't happen. I think a direct import should fix this for now btw ( |
Nice tip, now it works. This is the reason why it doesn't work without a direct import: facebook/metro#535 Metro wasn't able to import mjs files and the cascade imports are: @uppy/react -> @uppy/dashboard -> Dashboard.jsx -> thumbnail-generator -> index.js -> exifr/dist/mini.esm.mjs |
Great! so were does that leave us, any unresolved issues for now? |
Ok I'm missing one last thing which is to update the permissions handling with the new document picker and imagepicker expo libs! |
@giacomocerquone hi, do you have time to get this over the finish line? |
Yes, let me try to work on it during the holidays |
Socket Security Pull Request Report👍 No new dependency issues detected in pull request Pull request report summary
Bot CommandsTo ignore an alert, reply with a comment starting with Ignoring: Powered by socket.dev |
ok, this should be it... at least the first phase of putting the whole react-native integration back up is done. @Murderlon lemmeknow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this
Is there anything to be done about the "bin script confusion" mentioned above? The build is also failing: https://github.com/transloadit/uppy/actions/runs/3842153130/jobs/6549865412 |
ok the build seems to fail because, at a certain point in history, you grouped all the examples under the |
now the only thing that fails is this https://github.com/transloadit/uppy/actions/runs/3845686273/jobs/6550311324 which, running it locally, complaints about a lockfile change which is normal since we went from react-native-expo to @uppy-examples/react-native-expo console output
|
Did you perhaps forget to commit the updated |
I actually did Even running the install though, as you can see from the console output, yarn doesn't allow me to locally change the lock since the install fails. I really don't know yarn 3 much, is there a way to override this behaviour? For example like https://stackoverflow.com/a/67740771/2809729 UPDATE: Sorry, I was just reissuing the command launched by the ci that has the immutable flag :) Now I've correctly changed the lock launching the command locally without that flag |
The tests pass now. We only have that socket security warning. Not sure if we can do something about it or if we can ignore it? |
@SocketSecurity ignore @react-native-community/cli@5.0.1 |
@SocketSecurity ignore react-native@0.64.3 |
🎉 |
| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/audio | 1.0.3 | @uppy/locales | 3.0.5 | | @uppy/aws-s3 | 3.0.5 | @uppy/react | 3.1.0 | | @uppy/aws-s3-multipart | 3.1.2 | @uppy/react-native | 0.5.0 | | @uppy/companion | 4.2.0 | @uppy/transloadit | 3.1.0 | | @uppy/core | 3.0.5 | @uppy/utils | 5.1.2 | | @uppy/dashboard | 3.2.1 | uppy | 3.4.0 | - @uppy/utils: better fallbacks for the drag & drop API (Antoine du Hamel / #4260) - @uppy/core: fix metafields validation when used as function (Merlijn Vos / #4276) - @uppy/companion: allow customizing express session prefix (Mikael Finstad / #4249) - meta: Fix comment about COMPANION_PATH (Collin Allen / #4279) - @uppy/companion: Fix typo in KUBERNETES.md (Collin Allen / #4277) - @uppy/locales: update zh_TW.js (5idereal / #4270) - meta: ci: make sure Yarn's global cache is disabled (Antoine du Hamel / #4268) - @uppy/aws-s3-multipart: fix metadata shape (Antoine du Hamel / #4267) - meta: example: add multipart support to `aws-nodejs` (Antoine du Hamel / #4257) - @uppy/react-native: example: revive React Native example (Giacomo Cerquone / #4164) - @uppy/utils: Fix getSpeed type (referenced `bytesTotal` instead of `uploadStarted`) (Pascal Wengerter / #4263) - @uppy/companion: document how to run many instances (Mikael Finstad / #4227) - @uppy/aws-s3-multipart: add support for `allowedMetaFields` option (Antoine du Hamel / #4215) - meta: Fix indentation in generate-test.mjs (Youssef Victor / #4181) - @uppy/react: deprecate `useUppy` (Merlijn Vos / #4223) - meta: fix typo in README.md (Fuad Herac / #4254) - meta: Don’t close stale issues automatically (Artur Paikin / #4246) - meta: upgrade to Vite 4 and ESBuild 0.16 (Antoine du Hamel / #4243) - @uppy/audio: @uppy/audio fix typo in readme (elliotsayes / #4240) - @uppy/aws-s3: fix: add https:// to digital oceans link (Le Gia Hoang / #4165) - website: Simplify Dashboard code sample (Artur Paikin / #4197) - @uppy/transloadit: introduce `assemblyOptions`, deprecate other options (Merlijn Vos / #4059) - @uppy/core: fix typo in Uppy.test.js (Ikko Ashimine / #4235) - @uppy/aws-s3-multipart: fix singPart type (Stefan Schonert / #4224)
| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/audio | 1.0.3 | @uppy/locales | 3.0.5 | | @uppy/aws-s3 | 3.0.5 | @uppy/react | 3.1.0 | | @uppy/aws-s3-multipart | 3.1.2 | @uppy/react-native | 0.5.0 | | @uppy/companion | 4.2.0 | @uppy/transloadit | 3.1.0 | | @uppy/core | 3.0.5 | @uppy/utils | 5.1.2 | | @uppy/dashboard | 3.2.1 | uppy | 3.4.0 | - @uppy/utils: better fallbacks for the drag & drop API (Antoine du Hamel / transloadit#4260) - @uppy/core: fix metafields validation when used as function (Merlijn Vos / transloadit#4276) - @uppy/companion: allow customizing express session prefix (Mikael Finstad / transloadit#4249) - meta: Fix comment about COMPANION_PATH (Collin Allen / transloadit#4279) - @uppy/companion: Fix typo in KUBERNETES.md (Collin Allen / transloadit#4277) - @uppy/locales: update zh_TW.js (5idereal / transloadit#4270) - meta: ci: make sure Yarn's global cache is disabled (Antoine du Hamel / transloadit#4268) - @uppy/aws-s3-multipart: fix metadata shape (Antoine du Hamel / transloadit#4267) - meta: example: add multipart support to `aws-nodejs` (Antoine du Hamel / transloadit#4257) - @uppy/react-native: example: revive React Native example (Giacomo Cerquone / transloadit#4164) - @uppy/utils: Fix getSpeed type (referenced `bytesTotal` instead of `uploadStarted`) (Pascal Wengerter / transloadit#4263) - @uppy/companion: document how to run many instances (Mikael Finstad / transloadit#4227) - @uppy/aws-s3-multipart: add support for `allowedMetaFields` option (Antoine du Hamel / transloadit#4215) - meta: Fix indentation in generate-test.mjs (Youssef Victor / transloadit#4181) - @uppy/react: deprecate `useUppy` (Merlijn Vos / transloadit#4223) - meta: fix typo in README.md (Fuad Herac / transloadit#4254) - meta: Don’t close stale issues automatically (Artur Paikin / transloadit#4246) - meta: upgrade to Vite 4 and ESBuild 0.16 (Antoine du Hamel / transloadit#4243) - @uppy/audio: @uppy/audio fix typo in readme (elliotsayes / transloadit#4240) - @uppy/aws-s3: fix: add https:// to digital oceans link (Le Gia Hoang / transloadit#4165) - website: Simplify Dashboard code sample (Artur Paikin / transloadit#4197) - @uppy/transloadit: introduce `assemblyOptions`, deprecate other options (Merlijn Vos / transloadit#4059) - @uppy/core: fix typo in Uppy.test.js (Ikko Ashimine / transloadit#4235) - @uppy/aws-s3-multipart: fix singPart type (Stefan Schonert / transloadit#4224)
Wow, this monorepo is a bit of a nightmare sadly 😞
I'm so sorry for that lock but I've done everything correctly as requested by the contributing.md (using corepack, yarn 3.2.2 and so on) and I actually just moved a couple of dependencies.
So, continuing after what I commented on #3533 I'm trying to revive the react native implementation which is also a total nightmare.
In fact, this code I'm pushing does work in the sense that the following application starts, but then I'm encountering some error in uppy's core, regarding the usage of the tus browser wrapper that uses standard browser features unavailable to react-native of course:
App screens
What are your thoughts?