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.

Sign up for GitHub

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: async secureDeliveryProxyUrlResolver #677

Merged
merged 4 commits into from
Jul 16, 2024
Merged

feat: async secureDeliveryProxyUrlResolver #677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9181cea
feat: async `secureDeliveryProxyUrlResolver`
nd0ut Jun 20, 2024
e9e1a8b
feat: async `secureDeliveryProxyUrlResolver`
nd0ut Jun 21, 2024
3917397
refactor: use optional chaining to invoke fn
nd0ut Jun 25, 2024
f2c871e
Merge branch 'main' into feat/async-secure-delivery-callback
nd0ut Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions abstract/Block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,26 +236,30 @@ export class Block extends BaseComponent {
Data.deleteCtx(this.ctxName);

this.localeManager?.destroy();
this.a11y?.destroy();
}

/**
* @param {String} url
* @returns {String}
* @returns {Promise<String>}
* @protected
*/
proxyUrl(url) {
async proxyUrl(url) {
if (this.cfg.secureDeliveryProxy && this.cfg.secureDeliveryProxyUrlResolver) {
console.warn(
'Both secureDeliveryProxy and secureDeliveryProxyUrlResolver are set. The secureDeliveryProxyUrlResolver will be used.',
);
}
if (this.cfg.secureDeliveryProxyUrlResolver) {
return this.cfg.secureDeliveryProxyUrlResolver(url, {
uuid: extractUuid(url),
cdnUrlModifiers: extractCdnUrlModifiers(url),
fileName: extractFilename(url),
});
try {
return await this.cfg.secureDeliveryProxyUrlResolver(url, {
uuid: extractUuid(url),
cdnUrlModifiers: extractCdnUrlModifiers(url),
fileName: extractFilename(url),
});
} catch (err) {
console.error('Failed to resolve secure delivery proxy URL. Falling back to the default URL.', err);
return url;
}
}
if (this.cfg.secureDeliveryProxy) {
return applyTemplateData(
Expand Down
8 changes: 4 additions & 4 deletions abstract/UploaderBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,10 @@ export class UploaderBlock extends ActivityBlock {
return entry.getValue('errors').length > 0;
});
if (
uploadCollection.size > 0 &&
errorItems.length === 0 &&
uploadCollection.size === loadedItems.length &&
this.$['*collectionErrors'].length === 0
uploadCollection.size > 0 &&
errorItems.length === 0 &&
uploadCollection.size === loadedItems.length &&
this.$['*collectionErrors'].length === 0
) {
this.emit(
EventType.COMMON_UPLOAD_SUCCESS,
Expand Down
2 changes: 1 addition & 1 deletion blocks/CloudImageEditor/src/CloudImageEditorBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export class CloudImageEditorBlock extends Block {
}

try {
const cdnUrl = createCdnUrl(this.$['*originalUrl'], createCdnUrlModifiers('json'));
const cdnUrl = await this.proxyUrl(createCdnUrl(this.$['*originalUrl'], createCdnUrlModifiers('json')));
const json = await fetch(cdnUrl).then((response) => response.json());

const { width, height } = /** @type {{ width: number; height: number }} */ (json);
Expand Down
54 changes: 32 additions & 22 deletions blocks/CloudImageEditor/src/EditorFilterControl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@
// @ts-check
import { createCdnUrl, createCdnUrlModifiers } from '../../../utils/cdn-utils.js';
import { EditorButtonControl } from './EditorButtonControl.js';
import { FAKE_ORIGINAL_FILTER } from './EditorSlider.js';
import { COMMON_OPERATIONS, transformationsToOperations } from './lib/transformationUtils.js';
import { preloadImage } from './lib/preloadImage.js';

export class EditorFilterControl extends EditorButtonControl {
init$ = {
...this.init$,
active: false,
title: '',
icon: '',
isOriginal: false,
iconSize: '20',
'on.click': null,
};
constructor() {
super();

this.init$ = {
...this.init$,
active: false,
title: '',
icon: '',
isOriginal: false,
iconSize: '20',
'on.click': null,
};
}

_previewSrc() {
let previewSize = parseInt(window.getComputedStyle(this).getPropertyValue('--l-base-min-width'), 10);
Expand All @@ -24,6 +29,7 @@ export class EditorFilterControl extends EditorButtonControl {

/** @type {import('./types.js').Transformations} */
let transformations = { ...this.$['*editorTransformations'] };
// @ts-expect-error FIXME: fix this
Copy link
Contributor

Choose a reason for hiding this comment

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

Type assertion added for transformations.

The use of @ts-expect-error indicates that there is a known TypeScript issue here. It would be beneficial to resolve this issue rather than suppressing the error. If the type assertion is incorrect, it could lead to runtime errors.

- // @ts-expect-error FIXME: fix this
+ // TODO: Resolve type mismatch or provide a correct type assertion
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// @ts-expect-error FIXME: fix this
// TODO: Resolve type mismatch or provide a correct type assertion

transformations[this._operation] =
this._filter !== FAKE_ORIGINAL_FILTER
? {
Expand All @@ -47,10 +53,10 @@ export class EditorFilterControl extends EditorButtonControl {
* @param {IntersectionObserverEntry[]} entries
* @param {IntersectionObserver} observer
*/
_observerCallback(entries, observer) {
async _observerCallback(entries, observer) {
let intersectionEntry = entries[0];
if (intersectionEntry.isIntersecting) {
let src = this.proxyUrl(this._previewSrc());
let src = await this.proxyUrl(this._previewSrc());
let previewEl = this.ref['preview-el'];
let { promise, cancel } = preloadImage(src);
this._cancelPreload = cancel;
Expand All @@ -66,14 +72,14 @@ export class EditorFilterControl extends EditorButtonControl {
observer.unobserve(this);
});
} else {
this._cancelPreload && this._cancelPreload();
this._cancelPreload?.();
}
}

initCallback() {
super.initCallback();

this.$['on.click'] = (e) => {
this.$['on.click'] = () => {
if (!this.$.active) {
this.$['*sliderEl'].setOperation(this._operation, this._filter);
this.$['*sliderEl'].apply();
Expand All @@ -85,12 +91,16 @@ export class EditorFilterControl extends EditorButtonControl {
this.$['*currentFilter'] = this._filter;
};

this.defineAccessor('filter', (filter) => {
this._operation = 'filter';
this._filter = filter;
this.$.isOriginal = filter === FAKE_ORIGINAL_FILTER;
this.$.icon = this.$.isOriginal ? 'original' : 'slider';
});
this.defineAccessor(
'filter',
/** @param {string} filter */
(filter) => {
this._operation = 'filter';
this._filter = filter;
this.$.isOriginal = filter === FAKE_ORIGINAL_FILTER;
this.$.icon = this.$.isOriginal ? 'original' : 'slider';
},
);

this._observer = new window.IntersectionObserver(this._observerCallback.bind(this), {
threshold: [0, 1],
Expand Down Expand Up @@ -128,9 +138,9 @@ export class EditorFilterControl extends EditorButtonControl {
}
});

this.sub('*networkProblems', (networkProblems) => {
this.sub('*networkProblems', async (networkProblems) => {
if (!networkProblems) {
let src = this.proxyUrl(this._previewSrc());
let src = await this.proxyUrl(this._previewSrc());
let previewEl = this.ref['preview-el'];
if (previewEl.style.backgroundImage) {
previewEl.style.backgroundImage = 'none';
Expand All @@ -143,7 +153,7 @@ export class EditorFilterControl extends EditorButtonControl {
destroyCallback() {
super.destroyCallback();
this._observer?.disconnect();
this._cancelPreload && this._cancelPreload();
this._cancelPreload?.();
}
}

Expand Down
6 changes: 3 additions & 3 deletions blocks/CloudImageEditor/src/EditorImageCropper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ export class EditorImageCropper extends Block {
* @param {import('./types.js').Transformations} transformations
* @returns {Promise<HTMLImageElement>}
*/
_waitForImage(originalUrl, transformations) {
async _waitForImage(originalUrl, transformations) {
let width = this.offsetWidth;
transformations = {
...transformations,
Expand All @@ -470,13 +470,13 @@ export class EditorImageCropper extends Block {
flip: undefined,
mirror: undefined,
};
let src = this.proxyUrl(viewerImageSrc(originalUrl, width, transformations));
let src = await this.proxyUrl(viewerImageSrc(originalUrl, width, transformations));
let { promise, cancel, image } = preloadImage(src);

let stop = this._handleImageLoading(src);
image.addEventListener('load', stop, { once: true });
image.addEventListener('error', stop, { once: true });
this._cancelPreload && this._cancelPreload();
this._cancelPreload?.();
this._cancelPreload = cancel;

return promise
Expand Down
32 changes: 17 additions & 15 deletions blocks/CloudImageEditor/src/EditorImageFader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,9 @@ export class EditorImageFader extends Block {
* @param {String} [options.filter]
* @param {String} [options.operation]
* @param {Number} [options.value]
* @returns {String}
* @returns {Promise<String>}
*/
_imageSrc({ url = this._url, filter = this._filter, operation, value } = {}) {
async _imageSrc({ url = this._url, filter = this._filter, operation, value } = {}) {
let transformations = { ...this._transformations };

if (operation) {
Expand All @@ -149,17 +149,17 @@ export class EditorImageFader extends Block {

// do not use getBoundingClientRect because scale transform affects it
let width = this.offsetWidth;
return this.proxyUrl(viewerImageSrc(url, width, transformations));
return await this.proxyUrl(viewerImageSrc(url, width, transformations));
}

/**
* @private
* @param {String} operation
* @param {Number} value
* @returns {Keypoint}
* @returns {Promise<Keypoint>}
*/
_constructKeypoint(operation, value) {
let src = this._imageSrc({ operation, value });
async _constructKeypoint(operation, value) {
let src = await this._imageSrc({ operation, value });
Comment on lines +161 to +162
Copy link
Contributor

Choose a reason for hiding this comment

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

Review asynchronous methods _constructKeypoint and _addKeypoint for potential race conditions.

Both _constructKeypoint and _addKeypoint are now asynchronous and involve image loading and DOM manipulations. It's crucial to ensure that these methods handle race conditions, especially since they involve UI updates based on potentially outdated state due to asynchronous operations.

Consider adding checks or using techniques like debouncing or locking to ensure that the state used in these methods is still valid when the asynchronous operations complete.

Also applies to: 190-197

return {
src,
image: null,
Expand Down Expand Up @@ -187,14 +187,14 @@ export class EditorImageFader extends Block {
* @param {String | null} filter
* @param {Number} value
*/
_addKeypoint(operation, filter, value) {
async _addKeypoint(operation, filter, value) {
let shouldSkip = () =>
!this._isSame(operation, filter) || this._value !== value || !!this._keypoints.find((kp) => kp.value === value);

if (shouldSkip()) {
return;
}
let keypoint = this._constructKeypoint(operation, value);
let keypoint = await this._constructKeypoint(operation, value);
let image = new Image();
image.src = keypoint.src;
let stop = this._handleImageLoading(keypoint.src);
Expand Down Expand Up @@ -308,10 +308,10 @@ export class EditorImageFader extends Block {
}

/** @param {import('./types.js').Transformations} transformations */
setTransformations(transformations) {
async setTransformations(transformations) {
this._transformations = transformations;
if (this._previewImage) {
let src = this._imageSrc();
let src = await this._imageSrc();
Comment on lines +311 to +314
Copy link
Contributor

Choose a reason for hiding this comment

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

Verify error handling in setTransformations.

The method setTransformations now contains asynchronous operations but lacks explicit error handling for the await call.



This change adds error handling to the setTransformations method, ensuring that any exceptions thrown during the image source retrieval or event handling are caught and logged.




Committable suggestion was skipped due to low confidence.




    

  
  



    


        

      
  

    
  
    
    

  

  




        

      
    








    


  










    


  		



    

    

    

  
          let stop = this._handleImageLoading(src);






    

    

    

  
          this._previewImage.src = src;






    

    

    

  
          this._previewImage.addEventListener('load', stop, { once: true });






      

    
        
          
        
        Expand All
    
    @@ -335,11 +335,11 @@ export class EditorImageFader extends Block {
  


    

    

    

  
       * @param {Number} options.value






    

    

    

  
       * @param {String} [options.filter]






    

    

    

  
       */






    

    

    

  
      preload({ url, filter, operation, value }) {






    

    

    

  
      async preload({ url, filter, operation, value }) {






    

    

    

  
        this._cancelBatchPreload && this._cancelBatchPreload();






    

    

    

  
    







    

    

    

  
        let keypoints = keypointsRange(operation, value);






    

    

    

  
        let srcList = keypoints.map((kp) => this._imageSrc({ url, filter, operation, value: kp }));






    

    

    

  
        let srcList = await Promise.all(keypoints.map((kp) => this._imageSrc({ url, filter, operation, value: kp })));






      


  
        

      
    

    
    
          

      

      Comment on lines

    
      +338
     to 
      +342
    

  


  





  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      

  
    Contributor


      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
Add error handling to preload method.


The preload method is now asynchronous and involves preloading images based on dynamic URLs. It's important to add error handling to manage potential failures in image source generation or the preloading process.




This change introduces error handling to the preload method, ensuring that any exceptions thrown during the image source retrieval or the preloading process are caught and logged.




Committable suggestion was skipped due to low confidence.





Tools


Biome


[error] 339-339: Change to an optional chain.


Unsafe fix: Change to an optional chain.


(lint/complexity/useOptionalChain)






    

  
  



    


        

      
  

    
  
    
    

  

  




        

      
    








    


  










    


  		



    

    

    

  
        let { cancel } = batchPreloadImages(srcList);






    

    

    

  
    







    

    

    

  
        this._cancelBatchPreload = cancel;






      

    
          
            
          
          Expand Down
          
            
          
          Expand Up
    
    @@ -400,7 +400,7 @@ export class EditorImageFader extends Block {
  


    

    

    

  
       * @param {String} [options.filter]






    

    

    

  
       * @param {Boolean} [options.fromViewer]






    

    

    

  
       */






    

    

    

  
      activate({ url, operation, value, filter, fromViewer }) {






    

    

    

  
      async activate({ url, operation, value, filter, fromViewer }) {






    

    

    

  
        this._isActive = true;






    

    

    

  
        this._hidden = false;






    

    

    

  
        this._url = url;






      

    
        
          
        
        Expand All
    
    @@ -411,12 +411,14 @@ export class EditorImageFader extends Block {
  


    

    

    

  
    







    

    

    

  
        let isOriginal = typeof value !== 'number' && !filter;






    

    

    

  
        if (isOriginal) {






    

    

    

  
          let src = this._imageSrc({ operation, value });






    

    

    

  
          let src = await this._imageSrc({ operation, value });






    

    

    

  
          this._setOriginalSrc(src);






    

    

    

  
          this._container && this._container.remove();






    

    

    

  
          return;






    

    

    

  
        }






    

    

    

  
        this._keypoints = keypointsRange(operation, value).map((keyValue) => this._constructKeypoint(operation, keyValue));






    

    

    

  
        this._keypoints = await Promise.all(






    

    

    

  
          keypointsRange(operation, value).map((keyValue) => this._constructKeypoint(operation, keyValue)),






    

    

    

  
        );






    

    

    

  
    







    

    

    

  
        this._update(operation, value);






    

    

    

  
        this._initNodes();






  

    
          
            
          
          Expand Down
    
    
  



                
              


          


    

  



    
  

    

      

        


          

            
            
          


          



          
            8 changes: 4 additions & 4 deletions
          
          

        

  blocks/CloudImageEditor/src/EditorToolbar.js

  
  
      
      
  





        
      


      

        










            


          

          

            
              

                
    

              

            
            
                            

                            


                              
   View file



                                    


                                  


                            


                            

                            

    Open in desktop



                
            
          

        

      

    



    



          

            




              
      
    
              

                
                  

                      Original file line number
                      Diff line number
                      Diff line change
                  

                
                
                      
      

    
          
            
          
          Expand Up
    
    @@ -105,7 +105,7 @@ export class EditorToolbar extends Block {
  


    

    

    

  
            visible: 'uc-tab-toggles--visible',






    

    

    

  
          },






    

    

    

  
          'on.cancel': () => {






    

    

    

  
            this._cancelPreload && this._cancelPreload();






    

    

    

  
            this._cancelPreload?.();






    

    

    

  
            this.$['*on.cancel']();






    

    

    

  
          },






    

    

    

  
          'on.apply': () => {






      

    
          
            
          
          Expand Down
          
            
          
          Expand Up
    
    @@ -271,11 +271,11 @@ export class EditorToolbar extends Block {
  


    

    

    

  
      }






    

    

    

  
    







    

    

    

  
      /** @private */






    

    

    

  
      _preloadEditedImage() {






    

    

    

  
      async _preloadEditedImage() {






      


  
        

      
    

          

        
            

                nd0ut marked this conversation as resolved.
            


        
          Show resolved
          Hide resolved
      


    
        
          
  
    
    

        





    


  		



    

    

    

  
        if (this.$['*imgContainerEl'] && this.$['*originalUrl']) {






    

    

    

  
          let width = this.$['*imgContainerEl'].offsetWidth;






    

    

    

  
          let src = this.proxyUrl(viewerImageSrc(this.$['*originalUrl'], width, this.$['*editorTransformations']));






    

    

    

  
          this._cancelPreload && this._cancelPreload();






    

    

    

  
          let src = await this.proxyUrl(viewerImageSrc(this.$['*originalUrl'], width, this.$['*editorTransformations']));






    

    

    

  
          this._cancelPreload?.();






    

    

    

  
          let { cancel } = batchPreloadImages([src]);






    

    

    

  
          this._cancelPreload = () => {






    

    

    

  
            cancel();






  

    
          
            
          
          Expand Down
    
    
  



                
              


          


    

  



    
  

    

      

        


          

            
            
          


          



          
            2 changes: 1 addition & 1 deletion
          
          

        

  blocks/FileItem/FileItem.js

  
  
      
      
  





        
      


      

        










            


          

          

            
              

                
    

              

            
            
                            

                            


                              
   View file



                                    


                                  


                            


                            

                            

    Open in desktop



                
            
          

        

      

    



    



          

            




              
      
    
              

                
                  

                      Original file line number
                      Diff line number
                      Diff line change
                  

                
                
                      
      

    
          
            
          
          Expand Up
    
    @@ -132,7 +132,7 @@ export class FileItem extends UploaderBlock {
  


    

    

    

  
    







    

    

    

  
        if (entry.getValue('fileInfo') && entry.getValue('isImage')) {






    

    

    

  
          let size = this.cfg.thumbSize;






    

    

    

  
          let thumbUrl = this.proxyUrl(






    

    

    

  
          let thumbUrl = await this.proxyUrl(






    

    

    

  
            createCdnUrl(






    

    

    

  
              createOriginalUrl(this.cfg.cdnCname, this._entry.getValue('uuid')),






    

    

    

  
              createCdnUrlModifiers(entry.getValue('cdnUrlModifiers'), `scale_crop/${size}x${size}/center`),






  

    
          
            
          
          Expand Down
    
    
  



                
              


          


    

  



    
  

    

      

        


          

            
            
          


          



          
            2 changes: 1 addition & 1 deletion
          
          

        

  types/exported.d.ts

  
  
      
      
  





        
      


      

        










            


          

          

            
              

                
    

              

            
            
                            

                            


                              
   View file



                                    


                                  


                            


                            

                            

    Open in desktop



                
            
          

        

      

    



    



          

            




              
      
    
              

                
                  

                      Original file line number
                      Diff line number
                      Diff line change
                  

                
                
                      
      

    
          
            
          
          Expand Up
    
    @@ -15,7 +15,7 @@ export type LocaleDefinitionOverride = Record<string, LocaleDefinition>;
  


    

    

    

  
    export type SecureDeliveryProxyUrlResolver = (






    

    

    

  
      previewUrl: string,






    

    

    

  
      urlParts: { uuid: string; cdnUrlModifiers: string; fileName: string },






    

    

    

  
    ) => string;






    

    

    

  
    ) => Promise<string> | string;






    

    

    

  
    export type SecureUploadsSignatureAndExpire = { secureSignature: string; secureExpire: string };






    

    

    

  
    export type SecureUploadsSignatureResolver = () => Promise<SecureUploadsSignatureAndExpire | null>;






    

    

    

  
    export type IconHrefResolver = (iconName: string) => string;






  

    
          
            
          
          Expand Down
    
    
  



                
              


          


    

  



  



    

  
    
      Loading