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

Convert sets to arrays prior to using spread operator to enable transpilation to ES5 #3297

Merged
merged 1 commit into from
Nov 5, 2021

Conversation

johnmadair
Copy link
Contributor

This goes with: #3296

Transpiling Uppy 2 to ES5 results in errors due to the lack of polyfill support for the spread operator in ES5. Converting Sets to arrays prior to spreading fixes this and allows for Uppy 2 to be used in ES5 browsers.

@johnmadair johnmadair changed the title Convert sets to arrays prior to using spread operator to enable transpiration to ES5 Convert sets to arrays prior to using spread operator to enable transpilation to ES5 Nov 3, 2021
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Hi, note that we dropped support for 2.0.0 for older browsers that require a lot of polyfills. But you are saying there isn't a decent polyfill to fix this for you?

@Murderlon Murderlon linked an issue Nov 4, 2021 that may be closed by this pull request
@Murderlon Murderlon requested a review from aduh95 November 4, 2021 10:37
@johnmadair
Copy link
Contributor Author

Hi, note that we dropped support for 2.0.0 for older browsers that require a lot of polyfills. But you are saying there isn't a decent polyfill to fix this for you?

Yes, the lack of a polyfill for the spread operator for Set, specifically, due to the lack of Symbol.iterator prior to ES6.

I didn't do a comprehensive check, but it does seem that these are the only lines preventing Uppy 2 from working for ES5 browsers.

@arturi arturi merged commit a545c66 into transloadit:main Nov 5, 2021
@aduh95
Copy link
Contributor

aduh95 commented Nov 5, 2021

It's unclear to me how [...Array.from(new Set)] would not fail if [...new Set] fails, as I don't see how Array.from(new Set) can work if there is no iteration support.
Regarding polyfilling, doesn't core-js provide that? It looks like they say they do.

Murderlon added a commit that referenced this pull request Nov 8, 2021
* main:
  @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (#3079)
  Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297)
  Add showRecordingLength to webcam types (#3303)
  Required meta fields UI (#3285)
  Update BACKLOG.md
  Release [email protected]
  @uppy/[email protected]
@aduh95
Copy link
Contributor

aduh95 commented Nov 8, 2021

Here's how it's output in the legacy bundle (https://releases.transloadit.com/uppy/v2.2.1/uppy.legacy.js):

steps = [].concat(_classPrivateFieldLooseBase(this, _preProcessors)[_preProcessors], _classPrivateFieldLooseBase(this, _uploaders)[_uploaders], _classPrivateFieldLooseBase(this, _postProcessors)[_postProcessors]);

What seems to be happening is Babel replacing array spread operator with a concat operation, which would work if we were spreading arrays, but doesn't with Sets. Babel doing that is surprising (there's probably a safe settings that has been disabled at some point), not sure using Array.from is the correct solution, but at least it is a fix.

Murderlon added a commit that referenced this pull request Nov 10, 2021
* main:
  Remove references of incorrect `options` argument for `companion.socket` (#3307)
  Upgrade linting to 2.0.0-0 (#3280)
  Release [email protected], @uppy/[email protected]
  @uppy/[email protected]
  meta: update version references in docs
  Release @uppy/[email protected]
  @uppy/[email protected]
  @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (#3079)
  Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297)
  Add showRecordingLength to webcam types (#3303)
  Required meta fields UI (#3285)
  Update BACKLOG.md
  Release [email protected]
  @uppy/[email protected]
  fix: @uppy/utils - `getFileType()` always returns a string (#3294)
  Remove yarn file
  @uppy/companion: fix Dockerfiles (#3291)
  @uppy/companion: use Node.js 16.x for Dockerfile (#3289)
  if an item is checked, it can’t be disabled (#3288)
Murderlon added a commit that referenced this pull request Nov 11, 2021
* main: (30 commits)
  Refactor locale scripts & generate types and docs (#3276)
  Remove references of incorrect `options` argument for `companion.socket` (#3307)
  Upgrade linting to 2.0.0-0 (#3280)
  Release [email protected], @uppy/[email protected]
  @uppy/[email protected]
  meta: update version references in docs
  Release @uppy/[email protected]
  @uppy/[email protected]
  @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (#3079)
  Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297)
  Add showRecordingLength to webcam types (#3303)
  Required meta fields UI (#3285)
  Update BACKLOG.md
  Release [email protected]
  @uppy/[email protected]
  fix: @uppy/utils - `getFileType()` always returns a string (#3294)
  Remove yarn file
  @uppy/companion: fix Dockerfiles (#3291)
  @uppy/companion: use Node.js 16.x for Dockerfile (#3289)
  if an item is checked, it can’t be disabled (#3288)
  ...
@Murderlon Murderlon mentioned this pull request Nov 24, 2021
aduh95 added a commit that referenced this pull request Nov 24, 2021
aduh95 added a commit that referenced this pull request Nov 24, 2021
* Revert "Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (#3297)"

This reverts commit a545c66.

* build: disable loose transpilation for legacy bundle

Refs: #3297 (comment)
@jlowcs
Copy link

jlowcs commented Dec 9, 2021

For reference, this was reverted following this issue so we're back to a lack of ES5 support in the 2.3.0 release.

vymao pushed a commit to vymao/uppy that referenced this pull request Mar 29, 2022
* main: (30 commits)
  Refactor locale scripts & generate types and docs (transloadit#3276)
  Remove references of incorrect `options` argument for `companion.socket` (transloadit#3307)
  Upgrade linting to 2.0.0-0 (transloadit#3280)
  Release [email protected], @uppy/[email protected]
  @uppy/[email protected]
  meta: update version references in docs
  Release @uppy/[email protected]
  @uppy/[email protected]
  @uppy/aws-s3: use `Promise.allSettled` insead of custom utils (transloadit#3079)
  Convert sets to arrays prior to using spread operator to enable transpilation to ES5 (transloadit#3297)
  Add showRecordingLength to webcam types (transloadit#3303)
  Required meta fields UI (transloadit#3285)
  Update BACKLOG.md
  Release [email protected]
  @uppy/[email protected]
  fix: @uppy/utils - `getFileType()` always returns a string (transloadit#3294)
  Remove yarn file
  @uppy/companion: fix Dockerfiles (transloadit#3291)
  @uppy/companion: use Node.js 16.x for Dockerfile (transloadit#3289)
  if an item is checked, it can’t be disabled (transloadit#3288)
  ...
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.

Errors when transpiling Uppy 2.1.1 to ES5
5 participants