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

feat: always use https for loremflickr #1034

Merged
merged 2 commits into from
Jun 4, 2022
Merged

feat: always use https for loremflickr #1034

merged 2 commits into from
Jun 4, 2022

Conversation

bfanger
Copy link
Contributor

@bfanger bfanger commented Jun 3, 2022

When using https for you testpage you can only use the imageUrl function, all the other image helper functions will generate http:// urls.

This results in the image being blocked by the browser:

Mixed Content: The page at 'https://domain.tld/storybook/' was loaded over HTTPS,
but requested an insecure image 'http://loremflickr.com/100/100/nightlife?w=200&fm=webp&q=85'. 
This request has been blocked; the content must be served over HTTPS.

https image urls also work when the the page is hosted on http, in fact the http image urls will be redirected to https by lorumflicker.

This PR changes the default to https:// and because the image is served over https anyway i've removed the https?: boolean parameter instead of swapping the default value.

The only code change is in the imageUrl function( line 93), the other changes are inside comments.

@bfanger bfanger requested a review from a team as a code owner June 3, 2022 08:30
@Shinigami92 Shinigami92 added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Jun 3, 2022
@Shinigami92 Shinigami92 added this to the v7 - Current Major milestone Jun 3, 2022
@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #1034 (f5410d7) into main (9af77dd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1034   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files        2119     2119           
  Lines      227439   227432    -7     
  Branches      982      982           
=======================================
- Hits       226691   226687    -4     
+ Misses        728      725    -3     
  Partials       20       20           
Impacted Files Coverage Δ
src/modules/image/index.ts 100.00% <100.00%> (ø)
src/modules/finance/index.ts 100.00% <0.00%> (+0.68%) ⬆️

@Shinigami92 Shinigami92 requested review from a team June 3, 2022 08:47
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Should we not consider this a breaking change?

@ST-DDT ST-DDT requested a review from a team June 3, 2022 09:17
@Shinigami92 Shinigami92 changed the title feat: Default to https urls for images feat: always use https for loremflickr Jun 3, 2022
@Shinigami92
Copy link
Member

Should we not consider this a breaking change?

IMO it's okay, only very few customers would use this last parameter and even if they would use it, they only get a quick compile error in TS if they even have TS. In JS the last parameter would just be ignored.
I would count it as a minor breaking change, we will highlight it in next release notes at top.

@Shinigami92 Shinigami92 merged commit a235dca into faker-js:main Jun 4, 2022
Minozzzi pushed a commit to Minozzzi/faker that referenced this pull request Jul 19, 2022
@xDivisionByZerox xDivisionByZerox added the m: image Something is referring to the image module label Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: image Something is referring to the image module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants