Fix types and strictNullChecks #2009
Conversation
|
Thanks for the pr; I'll review it and hopefully merge it. |
|
Is there any progress review? |
|
Yes, should be done by Monday. |
src/js/gestures/gestures.js
Outdated
| @@ -317,14 +350,15 @@ class Gestures { | |||
| } | |||
|
|
|||
| this._updatePrevPoints(); | |||
| this.raf = requestAnimationFrame(this._rafRenderLoop.bind(this)); | |||
There was a problem hiding this comment.
Please do not rename props. I know it's tempting to rename vars that are named slightly _wrong, but it's an old script and ppl might be using them.
src/js/core/base.js
Outdated
| if (!dataSource) { | ||
| numItems = 0; | ||
| } else if ('length' in dataSource) { | ||
| dataSource = []; |
There was a problem hiding this comment.
dataSource may be undefined, no need to force it to be an array
There was a problem hiding this comment.
Fixed and updated types for events using dataSource
src/js/core/base.js
Outdated
|
|
||
| /** | ||
| * @protected | ||
| * @param {PhotoSwipeOptions} options | ||
| * @returns {PreparedPhotoSwipeOptions} | ||
| */ | ||
| _prepareOptions(options) { | ||
| if (window.matchMedia('(prefers-reduced-motion), (update: slow)').matches) { | ||
| options.showHideAnimationType = 'none'; | ||
| options.zoomAnimationDuration = 0; | ||
| } | ||
|
|
||
| /** @type {PreparedPhotoSwipeOptions} */ | ||
| return { | ||
| ...defaultOptions, | ||
| ...options | ||
| }; | ||
| } |
There was a problem hiding this comment.
_prepareOptions and defaultOptions should only exist after PhotoSwipe is loaded (within photoswipe.js).
Moving it to base.js makes it load twice and execute twice for no benefit.
src/js/core/eventable.js
Outdated
| */ | ||
|
|
||
| /** @type {PreparedPhotoSwipeOptions} */ | ||
| export const defaultOptions = { |
There was a problem hiding this comment.
defaultOptions should only exist after PhotoSwipe is loaded (within photoswipe.js).
src/js/core/eventable.js
Outdated
| /** @type {PhotoSwipeOptions} */ | ||
| this.options = undefined; | ||
| /** @type {PreparedPhotoSwipeOptions} */ | ||
| this.options = defaultOptions; |
src/js/slide/pan-bounds.js
Outdated
| @@ -82,19 +76,17 @@ class PanBounds { | |||
|
|
|||
| // _getZeroBounds | |||
| reset() { | |||
There was a problem hiding this comment.
Reuse objects, rather than recreating them.
| */ | ||
| getContentBySlide(slide) { | ||
| let content = this.getContentByIndex(slide.index); | ||
| if (!content) { | ||
| // create content if not found in cache | ||
| content = this.pswp.createContentFromData(slide.data, slide.index); | ||
| if (content) { |
There was a problem hiding this comment.
No. pswp.createContentFromData always returns instance of Content
src/js/slide/get-thumb-bounds.js
Outdated
| if (thumbnail) { | ||
| thumbnail = instance.applyFilters('thumbEl', thumbnail, itemData, index); |
There was a problem hiding this comment.
Allow filtering even if the thumbnail is missing initially
There was a problem hiding this comment.
fixed and updated types for filters
src/js/photoswipe.js
Outdated
| // initialize scroll wheel handler to block the scroll | ||
| this.scrollWheel = new ScrollWheel(this); | ||
| this.ui = new UI(this); |
There was a problem hiding this comment.
Moving Scrollwheel and UI initialization potentially breaks stuff.
Not only it breaks third-party components that rely on UI initializing later but also breaks the current wheel pain/zoom functionality.
Possibly ScrollWheel class needs a check to avoid such issues.
src/js/photoswipe.js
Outdated
| this.dispatch('change'); | ||
|
|
||
| this.opener.open(); | ||
|
|
||
| this.dispatch('afterInit'); | ||
|
|
||
| return true; |
|
Thanks for the work, I've merged the PR. Some tweaks might be required, but they can be added later. Hopefully, I'll rewrite everything to TS one day so the code is a bit cleaner and doesn't require separate files for types. |
This large PR contains:
This pr is tested in production on a large project. All photoswipe bugs in sentry have been fixed.