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/09/01 04:05:24 UTC
[GitHub] [incubator-superset] rusackas opened a new pull request #10750: chore: moving all @types dependencies to dev dependencies
rusackas opened a new pull request #10750:
URL: https://github.com/apache/incubator-superset/pull/10750
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
As per a chat with @etr2460 I figured it's worth trying to move all these to dev dependencies and see if CI explodes :)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
🤞 CI
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10750: chore: moving all @types dependencies to dev dependencies
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10750:
URL: https://github.com/apache/incubator-superset/pull/10750#issuecomment-684940998
My understanding is `@types/*` should always be in `dependencies` for libraries because you don't want your dependent package to not have out-of-the-box typing---it may be hard for library users to keep track of which additional `@types/*` are needed.
Since all NPM packages are presumed to be a "library" if they are going to be published, we always put `@types` in `dependencies` by default.
For the `superset-frontend` app though, or any other npm-managed frontend apps for that matter, is not going to be published as a library, so it's OK to put `@types` in either place. But still, it's probably preferred to put all dependencies in `dependencies`, since we may want `NODE_ENV=production npm install && npm build` to work as well.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on pull request #10750: chore: moving all @types dependencies to dev dependencies
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10750:
URL: https://github.com/apache/incubator-superset/pull/10750#issuecomment-684216648
Welp, this seems to _work_ so it passes the curiosity test. Wondering if @ktmud or @suddjian or anyone else with more miles clocked on their TypeScipt odometer would consider this PR a good or bad idea. ¯\\\_(ツ)_/¯
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas merged pull request #10750: chore: moving all @types dependencies to dev dependencies
Posted by GitBox <gi...@apache.org>.
rusackas merged pull request #10750:
URL: https://github.com/apache/incubator-superset/pull/10750
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on pull request #10750: chore: moving all @types dependencies to dev dependencies
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10750:
URL: https://github.com/apache/incubator-superset/pull/10750#issuecomment-685168860
> My understanding is `@types/*` should always be in `dependencies` for libraries because you don't want your dependent package to not have out-of-the-box typing---it may be hard for library users to keep track of which additional `@types/*` are needed.
>
> Since all NPM packages are presumed to be a "library" if they are going to be published, we always put `@types` in `dependencies` by default.
>
> For the `superset-frontend` app though, or any other npm-managed frontend apps for that matter, is not going to be published as a library, so it's OK to put `@types` in either place. In fact, one may even argue it's probably preferred to put all dependencies (including babel, webpack, jest, etc) in only one of `dependencies` or `devDependencies`, since
>
> 1. If we consider the source code the published product of the app, we may want `NODE_ENV=production npm install && npm build` to work as well. So `dependencies` is preferred.
> 2. If we consider the built assets the published product of the app, we may want to put them all in `devDependencies` for consistency and simplicity.
I'm inclined to go with #2 for the time being. We can always adjust later if the game changes. At least they should all be in one spot, to avoid further confusion, so it's a step in _A_ direction ;)
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud edited a comment on pull request #10750: chore: moving all @types dependencies to dev dependencies
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #10750:
URL: https://github.com/apache/incubator-superset/pull/10750#issuecomment-684940998
My understanding is `@types/*` should always be in `dependencies` for libraries because you don't want your dependent package to not have out-of-the-box typing---it may be hard for library users to keep track of which additional `@types/*` are needed.
Since all NPM packages are presumed to be a "library" if they are going to be published, we always put `@types` in `dependencies` by default.
For the `superset-frontend` app though, or any other npm-managed frontend apps for that matter, is not going to be published as a library, so it's OK to put `@types` in either place. In fact, one may even argue it's probably preferred to put all dependencies in only one of `dependencies` or `devDependencies`, since
1. If we consider the source code the published product of the app, we may want `NODE_ENV=production npm install && npm build` to work as well. So `dependencies` is preferred.
2. If we consider the built assets the published product of the app, we may want to put them all in `devDependencies` for consistency and simplicity.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud edited a comment on pull request #10750: chore: moving all @types dependencies to dev dependencies
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #10750:
URL: https://github.com/apache/incubator-superset/pull/10750#issuecomment-684940998
My understanding is `@types/*` should always be in `dependencies` for libraries because you don't want your dependent package to not have out-of-the-box typing---it may be hard for library users to keep track of which additional `@types/*` are needed.
Since all NPM packages are presumed to be a "library" if they are going to be published, we always put `@types` in `dependencies` by default.
For the `superset-frontend` app though, or any other npm-managed frontend apps for that matter, is not going to be published as a library, so it's OK to put `@types` in either place. In fact, one may even argue it's probably preferred to put all dependencies (including babel, webpack, jest, etc) in only one of `dependencies` or `devDependencies`, since
1. If we consider the source code the published product of the app, we may want `NODE_ENV=production npm install && npm build` to work as well. So `dependencies` is preferred.
2. If we consider the built assets the published product of the app, we may want to put them all in `devDependencies` for consistency and simplicity.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org