suddjian commented on a change in pull request #18250: URL: https://github.com/apache/superset/pull/18250#discussion_r797841377
########## File path: superset-embedded-sdk/CONTRIBUTING.md ########## @@ -0,0 +1,57 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +# Contributing to the Superset Embedded SDK + +The superset-embedded-sdk directory is a self contained sub-project in the Superset codebase. + +This is because the SDK has different requirements from other parts of the Superset codebase: +Namely, we need to export a lightweight frontend library that can be used in as many environments as possible. +Having separate configs allows for better separation of concerns and allows the SDK code to remain simple. + +## Building + +The library is built in two modes: one for consumption by package managers +and subsequent build systems, and one for consumption directly by a web browser. + +Babel is used to build the sdk into a relatively modern js package in the `lib` directory. +This is used by consumers who install the embedded sdk via npm, yarn, or other package manager. + +Webpack is used to bundle the `bundle` directory, +for use directly in the browser with no build step e.g. when importing via unpkg. + +Typescript outputs type definition files to the `dist` directory. + +Which of these outputs is used by the library consumer is determined by our package.json's `main`, `module`, and `types` fields. + +## Testing + +You may notice a lack of tests in this directory. The functions used in the sdk so far are very closely tied to browser behavior, Review comment: Hmm, I think you're right that it would be good to unit test the `fetchGuestToken` handling. And it would be nice to at least have one test to ensure that the code doesn't blow up. As for the others though, in my experience, unit testing helps most when you have branching logic in your code, and this code currently doesn't contain even one if statement. It just executes a series of instructions, all of which are interactions with external APIs. So, unit testing it beyond the "it doesn't blow up" case will essentially be asserting that the code exists as written. Those kinds of tests tend to just get in the way of changing the code instead of really helping much. I'll also add an "on pull request" github action that makes sure the build can still run. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
