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

Replace makeBlob with new Blob #3709

Merged
merged 8 commits into from
Oct 3, 2022
Merged

Replace makeBlob with new Blob #3709

merged 8 commits into from
Oct 3, 2022

Conversation

wch
Copy link
Collaborator

@wch wch commented Sep 30, 2022

The makeBlob function is no longer needed, since Blob has been available in all major browsers for a long time.

This PR was prompted by errors @jcheng5 encountered with a recent version of TypeScript, 4.8.4. As of version 4.4, MSBlobBuilder was removed from the types, which caused compilation errors.

This PR upgrades to TS 4.8.4, which resulted in a number of other packages needing to be upgraded.

Blob has long been available on all major browsers, so makeBlob is no longer needed.
@wch wch changed the title Replace makeBlob with Blob Replace makeBlob with new Blob Sep 30, 2022
@jcheng5
Copy link
Member

jcheng5 commented Sep 30, 2022

This looks awesome! By which I mean, I really want this to happen, but am not qualified to actually review the PR (clearly, since I spent a ton of time today trying to accomplish exactly this and got nowhere).

.eslintrc.yml Outdated
Comment on lines 110 to 114
- selector: objectLiteralProperty
format: null

- selector: objectLiteralMethod
format: null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why were these added? Example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One of the eslint package updates caused it to throw errors for existing code:


/.../shiny/srcts/src/bindings/input/fileinput.ts
   28:5  error  Object Literal Method name `dragenter.draghover` must match one of the following formats: camelCase        @typescript-eslint/naming-convention
   33:5  error  Object Literal Method name `dragleave.draghover` must match one of the following formats: camelCase        @typescript-eslint/naming-convention
   41:5  error  Object Literal Method name `dragover.draghover` must match one of the following formats: camelCase         @typescript-eslint/naming-convention
   44:5  error  Object Literal Method name `drop.draghover` must match one of the following formats: camelCase             @typescript-eslint/naming-convention
   59:5  error  Object Literal Method name `draghover:enter.draghover` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
   64:5  error  Object Literal Method name `draghover:leave.draghover` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
   69:5  error  Object Literal Method name `draghover:drop.draghover` must match one of the following formats: camelCase   @typescript-eslint/naming-convention
  257:7  error  Object Literal Method name `draghover:enter.draghover` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
  261:7  error  Object Literal Method name `draghover:leave.draghover` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
  267:7  error  Object Literal Method name `draghover:drop.draghover` must match one of the following formats: camelCase   @typescript-eslint/naming-convention

/.../shiny/srcts/src/imageutils/createBrush.ts
  393:9  error  Object Literal Property name `background-color` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
  395:9  error  Object Literal Property name `pointer-events` must match one of the following formats: camelCase    @typescript-eslint/naming-convention
  408:9  error  Object Literal Property name `border-left` must match one of the following formats: camelCase       @typescript-eslint/naming-convention
  409:9  error  Object Literal Property name `border-right` must match one of the following formats: camelCase      @typescript-eslint/naming-convention
  413:9  error  Object Literal Property name `border-top` must match one of the following formats: camelCase        @typescript-eslint/naming-convention
  414:9  error  Object Literal Property name `border-bottom` must match one of the following formats: camelCase     @typescript-eslint/naming-convention

/.../shiny/srcts/src/utils/index.ts
   8:5  error  Object Literal Property name `&` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
   9:5  error  Object Literal Property name `<` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
  10:5  error  Object Literal Property name `>` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
  12:5  error  Object Literal Property name `"` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
  13:5  error  Object Literal Property name `'` must match one of the following formats: camelCase  @typescript-eslint/naming-convention
  14:5  error  Object Literal Property name `/` must match one of the following formats: camelCase  @typescript-eslint/naming-convention

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm leaning towards we disable this lints for the two files or for the lines within each file, rather than globally.

// eslint-disable @typescript-eslint/naming-convention

or

// eslint-disable-next-line @typescript-eslint/naming-convention

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I learned that a rule can be disabled within a scope with the following (note that it needs /*-style comments, and not //):

/* eslint-disable @typescript-eslint/naming-convention */

So I've used that instead.

@wch wch merged commit 51da80d into main Oct 3, 2022
@wch wch deleted the blob branch October 3, 2022 17:36
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