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

Fix types and strictNullChecks #2009

Merged
merged 27 commits into from
Feb 1, 2023
Merged

Fix types and strictNullChecks #2009

merged 27 commits into from
Feb 1, 2023

Conversation

Tw1N88
Copy link
Contributor

@Tw1N88 Tw1N88 commented Jan 15, 2023

This large PR contains:

  1. strictNullChecks
  2. various type fixes according to docs
  3. code fixes according to types

This pr is tested in production on a large project. All photoswipe bugs in sentry have been fixed.

@dimsemenov
Copy link
Owner

Thanks for the pr; I'll review it and hopefully merge it.

@Tw1N88
Copy link
Contributor Author

Tw1N88 commented Jan 27, 2023

Is there any progress review?

@dimsemenov
Copy link
Owner

Yes, should be done by Monday.

Copy link
Owner

@dimsemenov dimsemenov left a comment

Choose a reason for hiding this comment

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

The direction of changes is good and type-related improvements are very useful. Some tweaks are needed, refer to comments. Thanks!

@@ -317,14 +350,15 @@ class Gestures {
}

this._updatePrevPoints();
this.raf = requestAnimationFrame(this._rafRenderLoop.bind(this));
Copy link
Owner

@dimsemenov dimsemenov Jan 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

if (!dataSource) {
numItems = 0;
} else if ('length' in dataSource) {
dataSource = [];
Copy link
Owner

Choose a reason for hiding this comment

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

dataSource may be undefined, no need to force it to be an array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and updated types for events using dataSource

Comment on lines 188 to 205

/**
* @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
};
}
Copy link
Owner

Choose a reason for hiding this comment

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

_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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

*/

/** @type {PreparedPhotoSwipeOptions} */
export const defaultOptions = {
Copy link
Owner

Choose a reason for hiding this comment

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

defaultOptions should only exist after PhotoSwipe is loaded (within photoswipe.js).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

/** @type {PhotoSwipeOptions} */
this.options = undefined;
/** @type {PreparedPhotoSwipeOptions} */
this.options = defaultOptions;
Copy link
Owner

Choose a reason for hiding this comment

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

options may be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -82,19 +76,17 @@ class PanBounds {

// _getZeroBounds
reset() {
Copy link
Owner

Choose a reason for hiding this comment

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

Reuse objects, rather than recreating them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

*/
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) {
Copy link
Owner

Choose a reason for hiding this comment

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

content may be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. pswp.createContentFromData always returns instance of Content

if (thumbnail) {
thumbnail = instance.applyFilters('thumbEl', thumbnail, itemData, index);
Copy link
Owner

Choose a reason for hiding this comment

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

Allow filtering even if the thumbnail is missing initially

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and updated types for filters

Comment on lines 293 to 295
// initialize scroll wheel handler to block the scroll
this.scrollWheel = new ScrollWheel(this);
this.ui = new UI(this);
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

this.dispatch('change');

this.opener.open();

this.dispatch('afterInit');

return true;
Copy link
Owner

Choose a reason for hiding this comment

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

This may be used, keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@dweber019
Copy link

This should fix #2003, #1926

@Tw1N88
Copy link
Contributor Author

Tw1N88 commented Feb 1, 2023

This should fix #2003, #1926

Will be great! )

@dimsemenov dimsemenov merged commit c33d150 into dimsemenov:master Feb 1, 2023
@dimsemenov
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants