Croppie icon indicating copy to clipboard operation
Croppie copied to clipboard

npm/webpack import has changed after 2.6.2 tag

Open zdennis opened this issue 5 years ago • 4 comments

On master there is a breaking change to how Croppie is to be imported when using npm/webpack. I see how Croppie exported has come up a bunch in #452, #470, #479, #487, #532, and #718. It looks like this breaking change may be intentional as a resolution for improving exports for the various module loading libraries out there, but I wanted to raise visibility that a release from master will require a change currently for anyone upgrading.

Expected Behavior

Naively, I would have assumed importing Croppie would not have broke.

Actual Behavior

After 9aefc6e828fd7b0fea42c36e82ac9cbaff6eb860 it looks like the import got broken. At the latest in master it appears that an upgrade will require a change from:

import { Croppie } from 'croppie/croppie';

to

import Croppie from 'croppie/croppie';`

I also see there is an open issue at https://github.com/DefinitelyTyped/DefinitelyTyped/issues/21928 . I am not sure if the current types would play a part in this breakage.

Steps to Reproduce the Problem

  1. Install [email protected] from npm and use on project

Example Link

TBD

Specifications

  • Browser:
  • Version:

zdennis avatar Nov 21 '18 16:11 zdennis

Thanks for pointing this out. I was going to make the next release 2.7.0 but maybe it should be 3.0.0. To indicate that it's a breaking change.

Personally I like the import Croppie from .... as opposed to import { Croppie } from..... We only recently switched to a more modern bundling system, so I didn't properly test the export changes that happened several months back like I should've.

In my ideal world, I'd have time to reorganize the library to something a little more maintainable, but that hasn't happened yet, hence the https://github.com/Foliotek/Croppie/tree/pre-2.7.0 branch (Although now it should probably be 3.0.0)

The DefinitelyTyped people are being patient with us, because the change has been in master for quite some time now.

thedustinsmith avatar Nov 21 '18 16:11 thedustinsmith

Personally I like the import Croppie from .... as opposed to import { Croppie } from..... We only recently switched to a more modern bundling system, so I didn't properly test the export changes that happened several months back like I should've.

One challenge with the current form is that it makes testing integration with Croppie more difficult. For example, in the import { Croppie } from.... form what's being exported is an object. This means that we can use a testing library like Jasmine to mock out Croppie and test basic interactions in an application, e.g.

// Here we refer to the object that contains Croppie as CroppieModule so 
// we can mock out Croppie in the test
import * as CroppieModule from ...

describe('NgxCroppieComponent', () => {
  beforeEach(() => {
    fakeCroppie = jasmine.createSpyObj('Croppie', ['bind', 'rotate', 'result', 'destroy']);
    spyOn(CroppieModule, 'Croppie').and.returnValue(fakeCroppie);
    // ...
  });

With the most recent form this is near impossible because what is exported is the constructor function for Croppie itself. The current form also precludes exporting other objects besides the constructor function.

If you're not opposed, I'd like to take a look at updating the module loading so it still exports an object to increase testability with projects that integrate with Croppie.

zdennis avatar Nov 21 '18 19:11 zdennis

I wonder why such backward-incompatible changes are done within this 2.6.x version. it should be released in v3

thg303 avatar Aug 02 '19 11:08 thg303

Why was this released in a patch version?!

leighman avatar Sep 24 '20 02:09 leighman