You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/02/20 19:35:55 UTC
[GitHub] [incubator-superset] etr2460 opened a new pull request #9180:
[SIP-36] Migrate setupApp.js to setupApp.ts
etr2460 opened a new pull request #9180: [SIP-36] Migrate setupApp.js to setupApp.ts
URL: https://github.com/apache/incubator-superset/pull/9180
### CATEGORY
Choose one
- [ ] Bug Fix
- [x] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
This is an example PR for migrating a JS file to TS. I performed the following steps:
1. Rename the file from `setupApp.js` => `setupApp.ts`
1. Install typedefs for imported libraries (ex `npm install @types/jquery` or add `// @ts-ignore` if no valid typedefs exist
1. Type function arguments to their expected values
1. Address all eslint warnings and errors
### TEST PLAN
CI, confirm that previous behavior is still correct. In this case, check changing locales still works and that checkboxes work in CRUD Views
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue: #9101
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
to: @nytai @graceguo-supercat @kristw @rusackas
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on issue #9180:
[SIP-36] Migrate setupApp.js to setupApp.ts
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9180: [SIP-36] Migrate setupApp.js to setupApp.ts
URL: https://github.com/apache/incubator-superset/pull/9180#issuecomment-589384968
i want to remove jQuery usage from superset-frontend... is it possible? https://www.sitepoint.com/jquery-document-ready-plain-javascript/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9180: [SIP-36]
Migrate setupApp.js to setupApp.ts
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9180: [SIP-36] Migrate setupApp.js to setupApp.ts
URL: https://github.com/apache/incubator-superset/pull/9180#issuecomment-589290292
That's a good point, any thoughts about how to test this file? The dependence on jquery makes me a bit uncertain about how to test....
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9180: [SIP-36]
Migrate setupApp.js to setupApp.ts
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9180: [SIP-36] Migrate setupApp.js to setupApp.ts
URL: https://github.com/apache/incubator-superset/pull/9180#issuecomment-589437062
++ on jquery removal as well. I don't think that's in scope for this PR though...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9180: [SIP-36]
Migrate setupApp.js to setupApp.ts
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9180: [SIP-36] Migrate setupApp.js to setupApp.ts
URL: https://github.com/apache/incubator-superset/pull/9180#issuecomment-589277061
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9180?src=pr&el=h1) Report
> Merging [#9180](https://codecov.io/gh/apache/incubator-superset/pull/9180?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/74423e5d19e78c3195547de94df78693fdce3061?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9180?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9180 +/- ##
=========================================
- Coverage 59.13% 59.1% -0.03%
=========================================
Files 372 372
Lines 11938 11943 +5
Branches 2925 2925
=========================================
Hits 7059 7059
- Misses 4699 4706 +7
+ Partials 180 178 -2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9180?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/setup/setupApp.ts](https://codecov.io/gh/apache/incubator-superset/pull/9180/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQXBwLnRz) | `0% <0%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9180?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9180?src=pr&el=footer). Last update [74423e5...07d6b7e](https://codecov.io/gh/apache/incubator-superset/pull/9180?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kristw merged pull request #9180: [SIP-36]
Migrate setupApp.js to setupApp.ts
Posted by GitBox <gi...@apache.org>.
kristw merged pull request #9180: [SIP-36] Migrate setupApp.js to setupApp.ts
URL: https://github.com/apache/incubator-superset/pull/9180
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on issue #9180: [SIP-36]
Migrate setupApp.js to setupApp.ts
Posted by GitBox <gi...@apache.org>.
nytai commented on issue #9180: [SIP-36] Migrate setupApp.js to setupApp.ts
URL: https://github.com/apache/incubator-superset/pull/9180#issuecomment-589413917
I reimplemented the menu in react, so it's possible to fully deprecate jquery. The react menu hasn't been propagated to all the pages, yet. I was hoping to progressively roll it out by replacing all the server rendered pages, however if someone wants to speed up jquery deprecation that's available.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on issue #9180:
[SIP-36] Migrate setupApp.js to setupApp.ts
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9180: [SIP-36] Migrate setupApp.js to setupApp.ts
URL: https://github.com/apache/incubator-superset/pull/9180#issuecomment-589289462
Since you are making migration to ts, could you add unit test for every new component? The overall test coverage seems keep going down.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org