fix unpslash author meta, sanitize metadata to strings and improve companion tests #3478

mifi · 2022-02-09T17:10:46Z

While working on #3477 I needed to make it easier to test the Uploader to create a regression test for testing the metadata. Once that issue is resolved and this is merged, we will be able to more easily write a test that confirms the metadata behavior for nested objects

all changes:

add test for xhr
improve validation logic - previously incorrect options gives an Uploader object in an incorrect state
fix root project jest: make sure we don't run companion tests on npm run test:unit (as they don't work in a browser/jsdom environment)
add npm run test:companion:watch
move env variables into code to make testing easier. this allows for watching (source is causing problems with npm scripts). also now we can run corepack yarn test:companion -t 'single test'
fix broken npm run test
~~Update:** have now also added validation of metadata to this PR (only allow string metadata values)~~
~~Update2: have also flattened the nested author from unsplash~~
Update3: stringify all metadata for form-data to prevent crash and add integration tests for it

add `npm run test:companion:watch` move env variables into code to make testing easier this allows for watching (source is causing problems with npm scripts) also now we can run `corepack yarn test:companion -t 'single test'`

make sure we don't run companion tests on npm run test:unit (as they don't work in a browser/jsdom environment)

previously incorrect options gives an Uploader object in an incorrect state

mifi · 2022-02-10T13:00:41Z

Ok the end-to-end-test fails, good! means it's working. Now need to fix this in the unsplash plugin

causing error #3477

arturi · 2022-02-14T16:05:32Z

related: form-data/form-data#137

# Conflicts: # packages/@uppy/provider-views/src/View.js

like the official FormData spec does

aduh95 · 2022-02-17T11:46:41Z

packages/@uppy/companion/src/server/Uploader.js

+  async tryUploadStream (stream) {
+    try {
+      const ret = await this.uploadStream(stream)
+      if (!ret) return


I'd like to pentition for us to start using nullish comparisons instead of falsy ones. Using falsy can be a footgun (because 0 and the empty string are falsy), and imho it's more readable and is more self-explanatory.

Suggested change

if (!ret) return

if (ret == null) return

I agree that nullish comparison is better when the function is expected to sometimes return a value where we consider 0 or '' to be valid values. In this case,if (ret == null) return would not return if ret by mistake is 0 or '', and it would continue on to the next line and crash, so I feel that !ret is "safer", as it will match more strictly. (it will return on more invalid values). do I make any sense?

I disagree it's "safer", I would argue the opposite: if the function returns 0 or an empty string it's almost certainly an error that we want to fix so I think a crash is more helpful than silently swallowing the type mismatch. Also, it wouldn't help if the function returns e.g. 1.
Anyway, if we really cared this kind of safety, we would write TypeScript (or make JSDoc mandatory) so I don't feel strongly about it, use the syntax that makes the more sense to you.

Actually when I think about it if we use if (ret == null) return, and ret is 0, then it would:

const { url, extraData } = 0

so it wouldn't crash, but it would emitSuccess(undefined, undefined)
Honestly I'm not sure what is best in this case, I just felt that !ret would be less likely to lead to strange behavior if ret was not an object, which is what we want in the success-case. Is there any way we can enforce x != null instead of !ret with eslint?

I suppose we could turn on an option to force to write == true for all conditions, but that would make checking for actual boolean value needlessly verbose, so I would be hesitant to turn on such option...

Ah ok. Then I think unless we can enforce it with jsdoc or TS like you say, I'm probably going to have a hard time adjusting to using that 😄 but I can try!

aduh95 · 2022-02-17T13:51:39Z

bin/companion.js

@@ -0,0 +1,2 @@
+require('dotenv').config({ path: '.env.local' })


That makes me think we should probably go back to using .env instead of .env.local. I thought .env.local was standard and would be picked up by dotenv by default, but it looks like it's not the case...

Ah, I thought we initially had a .env file too in addition to .env.local, although I cannot find it anywhere. Then I thought .env.local was local (dev) overrides for .env. That is how I'm used to it working in create-react-app.
But unless we want to commit a .env with default values, then I agree we can instead just use .env, because it doesn't seem like dotenv supports .env.local

Yes, my original idea was to have a .env file that would contain the default values, and have a .env.local file that would override it. I gave up on that and went back on having only one file because:

dotenv docs explicitly recommend against that: https://github.com/motdotla/dotenv#should-i-have-multiple-env-files.

It made overriding config for the e2e CI tricky.

(It looks like Merlijn went through a similar train of thought as he also settled on using only .env on his own)

Let me open a PR to address that.

Ah. Yes dotenv also recommends against commiting .env files or env variables. Same with the 12 factor app. But then dotenv is more geared towards backend node.js apps where .env files contain actual secrets. create-react-app on the other hand does recommend commiting .env files. But then on the flipside because frontend apps will usually expose all env variables in the bundled code anyways, it doesn't make sense to try to keep them secret. Our .env is a mix between backend and frontend, so then who knows what's right 🤷‍♂️

But I think keeping .env out of git is the only safe common denominator

aduh95 · 2022-02-17T15:03:19Z

Looks like the merge commit did some weird stuff, can you do:

git rm bin/companion.js
git checkout main -- README.md

mifi · 2022-02-17T15:07:00Z

Oh yea I noticed that too. I think it's due to the fact that we have a pre-commit hook that mutates files right before commiting:

uppy/package.json

Line 9 in c2692e4

"*.md": [

I think we should remove all the --fix and the commands that will mutate, what do you think?

mifi · 2022-02-17T15:12:31Z

Yea it keep modifying readme.md so even after running git checkout main -- README.md and commiting, it will change it back and remove it from the commit

aduh95 · 2022-02-17T15:16:13Z

Nah let's keep those --fix, it's less headache to have them than not having them. Why non-fixed code was pushed in the first place is something we need to fix. Probably something related to the release automation is broken.

mifi · 2022-02-17T15:16:34Z

I removed all --fix from eslint and -o from remark and now it works

mifi · 2022-02-17T15:17:49Z

I absolutely agree that we should keep precommit hooks to catch lint errors before commiting, but I don't think they should modify code silently before commiting. It can lead to strange things like this when merging.

aduh95 · 2022-02-17T15:17:57Z

Can you git revert 6eaa516473d4e70a0053b1b4559075a8acacdd0d please?

aduh95 · 2022-02-17T15:21:38Z

I absolutely agree that we should keep precommit hooks to catch lint errors before commiting, but I don't think they should modify code silently before commiting. It can lead to strange things like this when merging.

I guess it could be discussed, but it has little to do with unsplash and should go in its own PR.
FWIW I don't think it's a strong enough case for remove --fix, we have more to gain than to lose from this automatic code fixes IMHO.

mifi · 2022-02-17T15:23:42Z

true, I can create a separate issue about it. but I'm afraid if I revert it now, then readme will be modified again

mifi · 2022-02-17T15:26:18Z

ok I think I managed to push it without running the commit hooks first!

aduh95 · 2022-02-17T15:29:10Z

I have a fix PR for the README formatting: #3499

package.json

website/src/docs/migration-guides.md

| Package | Version | Package | Version | | --------------- | ------- | --------------- | ------- | | @uppy/companion | 3.3.0 | uppy | 2.6.0 | | @uppy/robodog | 2.3.2 | | | - meta: warn about not merging PR manually (Artur Paikin / #3492) - @uppy/companion: fix unpslash author meta, sanitize metadata to strings and improve companion tests (Mikael Finstad / #3478) - meta: ensure README is correctly formatted when doing releases (Antoine du Hamel / #3499) - meta: fix CDN bundle (Antoine du Hamel / #3494) - meta: fix missing EOL and end of e2e test templates (Antoine du Hamel / #3484) - meta: use a single `.env` file for config (Antoine du Hamel / #3498)

…mpanion tests (transloadit#3478) * fix broken npm run test * improve jest / companion add `npm run test:companion:watch` move env variables into code to make testing easier this allows for watching (source is causing problems with npm scripts) also now we can run `corepack yarn test:companion -t 'single test'` * fix root project jest make sure we don't run companion tests on npm run test:unit (as they don't work in a browser/jsdom environment) * improve validation logic previously incorrect options gives an Uploader object in an incorrect state * rewrite uploader to make it more testable * add test for xhr * check that metadata values are strings * fix nested meta causing error transloadit#3477 * convert meta to strings instead like the official FormData spec does * fix broken companion dev transloadit#3473 * fix botched merge * fix botched merge and remove --fix * fix botchedf merge * quick fix * . * remove eslint fix

| Package | Version | Package | Version | | --------------- | ------- | --------------- | ------- | | @uppy/companion | 3.3.0 | uppy | 2.6.0 | | @uppy/robodog | 2.3.2 | | | - meta: warn about not merging PR manually (Artur Paikin / transloadit#3492) - @uppy/companion: fix unpslash author meta, sanitize metadata to strings and improve companion tests (Mikael Finstad / transloadit#3478) - meta: ensure README is correctly formatted when doing releases (Antoine du Hamel / transloadit#3499) - meta: fix CDN bundle (Antoine du Hamel / transloadit#3494) - meta: fix missing EOL and end of e2e test templates (Antoine du Hamel / transloadit#3484) - meta: use a single `.env` file for config (Antoine du Hamel / transloadit#3498)

mifi added 7 commits February 9, 2022 21:54

fix broken npm run test

2b6e4e8

improve jest / companion

b229f1d

add `npm run test:companion:watch` move env variables into code to make testing easier this allows for watching (source is causing problems with npm scripts) also now we can run `corepack yarn test:companion -t 'single test'`

fix root project jest

00f7e4a

make sure we don't run companion tests on npm run test:unit (as they don't work in a browser/jsdom environment)

improve validation logic

9614192

previously incorrect options gives an Uploader object in an incorrect state

rewrite uploader to make it more testable

6c75c1a

add test for xhr

2e9cc52

check that metadata values are strings

6415854

mifi changed the title ~~Improve companion tests~~ Check that metadata values are strings and improve companion tests Feb 10, 2022

fix nested meta

29ae919

causing error #3477

mifi changed the title ~~Check that metadata values are strings and improve companion tests~~ fix unpslash author meta, validate metadata and improve companion tests Feb 10, 2022

mifi mentioned this pull request Feb 10, 2022

companion source.on is not a function error with XHR from Unsplash #3477

Closed

1 task

mifi requested a review from aduh95 February 12, 2022 16:00

arturi mentioned this pull request Feb 14, 2022

@uppy/unsplash: fix nested meta #3485

Merged

mifi added 2 commits February 16, 2022 21:59

Merge branch 'main' into improve-companion-tests

102bf83

# Conflicts: # packages/@uppy/provider-views/src/View.js

convert meta to strings instead

c744ab9

like the official FormData spec does

mifi requested a review from arturi February 16, 2022 16:17

mifi changed the title ~~fix unpslash author meta, validate metadata and improve companion tests~~ fix unpslash author meta, sanitize metadata to strings and improve companion tests Feb 17, 2022

mifi mentioned this pull request Feb 17, 2022

Remove request and form-data from companion #3496

Closed

aduh95 reviewed Feb 17, 2022

View reviewed changes

fix broken companion dev #3473

57f6c78

aduh95 reviewed Feb 17, 2022

View reviewed changes

aduh95 mentioned this pull request Feb 17, 2022

meta: use a single .env file for config #3498

Merged

Merge branch 'main' into improve-companion-tests

cff53e1

fix botched merge

46e085a

fix botched merge and remove --fix

6eaa516

fix botchedf merge

cc7b9a9

quick fix

bb63391

aduh95 mentioned this pull request Feb 17, 2022

meta: ensure README is correctly formatted when doing releases #3499

Merged

mifi mentioned this pull request Feb 17, 2022

meta: proposal: remove code mutation in precommit hooks #3500

Closed

aduh95 reviewed Feb 17, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

aduh95 reviewed Feb 17, 2022

View reviewed changes

website/src/docs/migration-guides.md Outdated Show resolved Hide resolved

mifi added 2 commits February 17, 2022 23:56

.

2048262

remove eslint fix

8cf4537

aduh95 approved these changes Feb 17, 2022

View reviewed changes

arturi merged commit 5cfb9fe into main Feb 17, 2022

arturi deleted the improve-companion-tests branch February 17, 2022 17:43

github-actions bot mentioned this pull request Feb 17, 2022

Release: [email protected] #3501

Merged

snyk-bot mentioned this pull request Mar 3, 2022

[Snyk] Security upgrade @uppy/companion from 0.17.5 to 3.3.1 H4NG-MAN/uppy#93

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix unpslash author meta, sanitize metadata to strings and improve companion tests #3478

fix unpslash author meta, sanitize metadata to strings and improve companion tests #3478

mifi commented Feb 9, 2022 •

edited

Loading

mifi commented Feb 10, 2022

arturi commented Feb 14, 2022

aduh95 Feb 17, 2022

mifi Feb 17, 2022

aduh95 Feb 17, 2022

mifi Feb 17, 2022

aduh95 Feb 17, 2022

mifi Feb 17, 2022

aduh95 Feb 17, 2022

mifi Feb 17, 2022

aduh95 Feb 17, 2022 •

edited

Loading

mifi Feb 17, 2022

aduh95 commented Feb 17, 2022

mifi commented Feb 17, 2022

mifi commented Feb 17, 2022

aduh95 commented Feb 17, 2022

mifi commented Feb 17, 2022

mifi commented Feb 17, 2022

aduh95 commented Feb 17, 2022

aduh95 commented Feb 17, 2022

mifi commented Feb 17, 2022

mifi commented Feb 17, 2022

aduh95 commented Feb 17, 2022

		@@ -0,0 +1,2 @@
		require('dotenv').config({ path: '.env.local' })

fix unpslash author meta, sanitize metadata to strings and improve companion tests #3478

fix unpslash author meta, sanitize metadata to strings and improve companion tests #3478

Conversation

mifi commented Feb 9, 2022 • edited Loading

mifi commented Feb 10, 2022

arturi commented Feb 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 commented Feb 17, 2022

mifi commented Feb 17, 2022

mifi commented Feb 17, 2022

aduh95 commented Feb 17, 2022

mifi commented Feb 17, 2022

mifi commented Feb 17, 2022

aduh95 commented Feb 17, 2022

aduh95 commented Feb 17, 2022

mifi commented Feb 17, 2022

mifi commented Feb 17, 2022

aduh95 commented Feb 17, 2022

mifi commented Feb 9, 2022 •

edited

Loading

aduh95 Feb 17, 2022 •

edited

Loading