Feature/typescript support #3
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows for users to pass their server functions as a type parameter
to the server class. This can be done by passing the tyepof an object
containing the server functions (their form when importing them from an
index).
import Server from 'gas-client';
import * as serverFunctions from '../server';
const { serverFunctions } = new Server<typeof serverFunctions>();
Prevents users from reassigning serverFunctions and allows config to be reused in other methods.
Allows the use of optional parameter and creates removal of undefined type
Suggestion: Since it's using TypeScript, there's not much use for Babel.
Suggestion/request: I am using this in an Angular App and the compiler doesn't pass the node variables to the file, which results in skipping the proxy setup, since process is not defined. Webpack does provide a way to mock or even actually pass the variables to the file, but I couldn't think of some situation where both the error messages and the production env would need to be checked. I think that just checking the message would suffice and allow for more flexible setups.
From all the types, I think exporting this would be useful for cases
where users declare a typed serverFunctions variable.
import {GasClient, ServerFunctions} from 'gas-client';
const {serverFunctions}: ServerFunctions = new GasClient();
Use GAS environment to decide if it will setup promises or a proxy. Allows for unpredicted errors to be thrown so we'll know if something is broken.
Since the project is using TypeScript, having 80 print width results in some very strange line breaks
enuchi
reviewed
Sep 14, 2020
There was a problem hiding this comment.
See some of my comments in this review. Overall this is great -- big improvements. Thanks for taking the time to open the PR.
I tried adding this in to my React starter project, replacing the following code in https://github.com/enuchi/React-Google-Apps-Script/blob/master/src/client/utils/server.js, and changing the file to a .ts file, and it worked well:
import { GASClient } from 'gas-client';
import * as sheetFunctions from '../../server/sheets';
const { PORT } = process.env;
const server = new GASClient<typeof sheetFunctions>({
allowedDevelopmentDomains: `https://localhost:${PORT}`,
});Even though the project is mainly javascript, there is pretty good type inference since it uses the Google Apps Script types package on the server. It's great getting type hints on the client, even in a javascript project.
I'm wondering if you know an easy way to use jsdoc to pass in the server method types. Essentially, is there an equivalent for new GASClient<typeof ...> in jsdoc? Would be nice to get type support for javascript users without using a typescript file. I got something to work but it was pretty complicated...
Thanks again.
| export { getSheetData, appendRowsToSheet }; | ||
| ``` | ||
|
|
||
| ### On your server-side code |
There was a problem hiding this comment.
I think this should be "client-side"
| }), | ||
| ]; | ||
| ``` | ||
| Development mode for the `gas-client` helper class will be run when the `google` client API cannot be found, i.e. an error is thrown with the message "google is not defined" |
There was a problem hiding this comment.
This line should probably be updated to match your new condition (no longer by throwing error).
There was a problem hiding this comment.
How about something like
Development mode for the
gas-clienthelper class will be run when thegoogle.script.rundoes not exist.
| @@ -1,42 +1,45 @@ | |||
| { | |||
| "name": "gas-client", | |||
| "name": "@guilhermetod/gas-client", | |||
There was a problem hiding this comment.
For this PR can you change back to the original?
| ], | ||
| "repository": "https://github.com/enuchi/gas-client.git", | ||
| "repository": "https://github.com/guilhermetod/gas-client.git", |
There was a problem hiding this comment.
For this PR can you change back to the original?
| @@ -1,42 +1,45 @@ | |||
| { | |||
| "name": "gas-client", | |||
| "name": "@guilhermetod/gas-client", | |||
| "version": "0.1.0", | |||
There was a problem hiding this comment.
This should probably be bumped to new major version 1.0.0 since it will be a breaking change.
| /** | ||
| * google.script.history is an asynchronous client-side JavaScript API that can interact with the browser history stack. It can only be used in the context of a web app that uses IFRAME. | ||
| */ | ||
| namespace history { |
There was a problem hiding this comment.
Are you using all these additional params (like history) or are these just copied from that other source?
| @@ -0,0 +1,3 @@ | |||
| const isGASEnvironment = (): boolean => typeof google !== 'undefined' && Boolean(google?.script?.run); | |||
There was a problem hiding this comment.
Thanks -- this is a better option than checking the error message. I still wonder if there are other some possible situations in production where google won't load and google.script.run cannot be found, in which case it will default to the Proxy "dev" mode and fire off some unwanted iframe messages. I added the environment variable so it would be more explicit which environment is desired, but I understand that it adds an annoying extra configuration with webpack. Anyway, overall I agree this is probably sufficient.
There was a problem hiding this comment.
By the way, one upside to using webpack to set up environment variables is that webpack can know at build time whether to use the proxy (dev) or the promisified (prod) version, and only include that code. With the new approach, all the proxy and iframe event code is getting bundled in both dev and prod builds since it only knows at runtime which environment it's in. Any thoughts on how to remove the unneeded code in prod builds?
There was a problem hiding this comment.
I left this as is in #19. There was actually an issue with Safari with the way I was doing it based on the error message thrown (Safari had a different error message 😁) so the way you did it works better.
| @@ -4,8 +4,7 @@ | |||
| "description": "A client-side utility class that can call server-side Google Apps Script functions", | |||
| "main": "build/index.js", | |||
| "files": [ | |||
| "build", | |||
| "src" | |||
| "build" | |||
There was a problem hiding this comment.
I prefer keeping src in the bundle. It's nice to be able to inspect the src code easily.
|
I am going to point this PR to a separate branch. |
|
Hey @enuchi I'm really sorry I left this here with no response. I've been really rushing on the last semester of college and finishing some other projects. Feel free to make any modifications you want in the PR. I also apologize for letting me as the author. I was testing the npm deploy in a scoped package and forgot to revert it before submitting. If you need help, I'll probably be free by around December 10th |
enuchi
pushed a commit
that referenced
this pull request
May 30, 2021
* Build: setup TypeScript
* Feat: add google types
* Feat: add server types
* Feat: transformed code into TypeScript
This allows for users to pass their server functions as a type parameter
to the server class. This can be done by passing the tyepof an object
containing the server functions (their form when importing them from an
index).
import Server from 'gas-client';
import * as serverFunctions from '../server';
const { serverFunctions } = new Server<typeof serverFunctions>();
* Refactor: check env with optional chaining
* Refactor: make other properties private
Prevents users from reassigning serverFunctions and allows config to be
reused in other methods.
* Refactor: change checkAllowList parameter order
Allows the use of optional parameter and creates removal of undefined
type
* Refactor: move constants to specific files
* Refactor: create promisify function
* Refactor: create shouldSetupProxy function
* Refactor: split constructor code in methods
* Refactor: merge multiple return checks
* Build: Remove Babel
Suggestion: Since it's using TypeScript, there's not much use for Babel.
* Refactor: use error message instead of toString
* Refactor: don't require process.env to setup proxy
Suggestion/request: I am using this in an Angular App and the compiler
doesn't pass the node variables to the file, which results in skipping
the proxy setup, since process is not defined. Webpack does provide a
way to mock or even actually pass the variables to the file, but I
couldn't think of some situation where both the error messages and the
production env would need to be checked. I think that just checking the
message would suffice and allow for more flexible setups.
* Style: change class name
* Feature: export ServerFunctions type
From all the types, I think exporting this would be useful for cases
where users declare a typed serverFunctions variable.
import {GasClient, ServerFunctions} from 'gas-client';
const {serverFunctions}: ServerFunctions = new GasClient();
* Docs: update readme
* Docs: fix typos
* Build: remove src from package files
* Style: fix acronym case
* Refactor: change try-catch block to if-else
Use GAS environment to decide if it will setup promises or a proxy.
Allows for unpredicted errors to be thrown so we'll know if something
is broken.
* Style: change type names
* Refactor: move function setup to dedicated classes
* Style: change prettier print width to 120
Since the project is using TypeScript, having 80 print width results in
some very strange line breaks
* Fix: correctly scope window interfaces
* Feature: create dev server types to export
* Docs: setup custom repository and registry
* Style: standardize imports and exports
* Style: remove comments and JSDocs
* Fix: change global types to exports
enuchi
pushed a commit
that referenced
this pull request
May 30, 2021
* Build: setup TypeScript
* Feat: add google types
* Feat: add server types
* Feat: transformed code into TypeScript
This allows for users to pass their server functions as a type parameter
to the server class. This can be done by passing the tyepof an object
containing the server functions (their form when importing them from an
index).
import Server from 'gas-client';
import * as serverFunctions from '../server';
const { serverFunctions } = new Server<typeof serverFunctions>();
* Refactor: check env with optional chaining
* Refactor: make other properties private
Prevents users from reassigning serverFunctions and allows config to be
reused in other methods.
* Refactor: change checkAllowList parameter order
Allows the use of optional parameter and creates removal of undefined
type
* Refactor: move constants to specific files
* Refactor: create promisify function
* Refactor: create shouldSetupProxy function
* Refactor: split constructor code in methods
* Refactor: merge multiple return checks
* Build: Remove Babel
Suggestion: Since it's using TypeScript, there's not much use for Babel.
* Refactor: use error message instead of toString
* Refactor: don't require process.env to setup proxy
Suggestion/request: I am using this in an Angular App and the compiler
doesn't pass the node variables to the file, which results in skipping
the proxy setup, since process is not defined. Webpack does provide a
way to mock or even actually pass the variables to the file, but I
couldn't think of some situation where both the error messages and the
production env would need to be checked. I think that just checking the
message would suffice and allow for more flexible setups.
* Style: change class name
* Feature: export ServerFunctions type
From all the types, I think exporting this would be useful for cases
where users declare a typed serverFunctions variable.
import {GasClient, ServerFunctions} from 'gas-client';
const {serverFunctions}: ServerFunctions = new GasClient();
* Docs: update readme
* Docs: fix typos
* Build: remove src from package files
* Style: fix acronym case
* Refactor: change try-catch block to if-else
Use GAS environment to decide if it will setup promises or a proxy.
Allows for unpredicted errors to be thrown so we'll know if something
is broken.
* Style: change type names
* Refactor: move function setup to dedicated classes
* Style: change prettier print width to 120
Since the project is using TypeScript, having 80 print width results in
some very strange line breaks
* Fix: correctly scope window interfaces
* Feature: create dev server types to export
* Docs: setup custom repository and registry
* Style: standardize imports and exports
* Style: remove comments and JSDocs
* Fix: change global types to exports
enuchi
pushed a commit
that referenced
this pull request
Jun 3, 2021
* Build: setup TypeScript
* Feat: add google types
* Feat: add server types
* Feat: transformed code into TypeScript
This allows for users to pass their server functions as a type parameter
to the server class. This can be done by passing the tyepof an object
containing the server functions (their form when importing them from an
index).
import Server from 'gas-client';
import * as serverFunctions from '../server';
const { serverFunctions } = new Server<typeof serverFunctions>();
* Refactor: check env with optional chaining
* Refactor: make other properties private
Prevents users from reassigning serverFunctions and allows config to be
reused in other methods.
* Refactor: change checkAllowList parameter order
Allows the use of optional parameter and creates removal of undefined
type
* Refactor: move constants to specific files
* Refactor: create promisify function
* Refactor: create shouldSetupProxy function
* Refactor: split constructor code in methods
* Refactor: merge multiple return checks
* Build: Remove Babel
Suggestion: Since it's using TypeScript, there's not much use for Babel.
* Refactor: use error message instead of toString
* Refactor: don't require process.env to setup proxy
Suggestion/request: I am using this in an Angular App and the compiler
doesn't pass the node variables to the file, which results in skipping
the proxy setup, since process is not defined. Webpack does provide a
way to mock or even actually pass the variables to the file, but I
couldn't think of some situation where both the error messages and the
production env would need to be checked. I think that just checking the
message would suffice and allow for more flexible setups.
* Style: change class name
* Feature: export ServerFunctions type
From all the types, I think exporting this would be useful for cases
where users declare a typed serverFunctions variable.
import {GasClient, ServerFunctions} from 'gas-client';
const {serverFunctions}: ServerFunctions = new GasClient();
* Docs: update readme
* Docs: fix typos
* Build: remove src from package files
* Style: fix acronym case
* Refactor: change try-catch block to if-else
Use GAS environment to decide if it will setup promises or a proxy.
Allows for unpredicted errors to be thrown so we'll know if something
is broken.
* Style: change type names
* Refactor: move function setup to dedicated classes
* Style: change prettier print width to 120
Since the project is using TypeScript, having 80 print width results in
some very strange line breaks
* Fix: correctly scope window interfaces
* Feature: create dev server types to export
* Docs: setup custom repository and registry
* Style: standardize imports and exports
* Style: remove comments and JSDocs
* Fix: change global types to exports
enuchi
pushed a commit
that referenced
this pull request
Jun 3, 2021
* Build: setup TypeScript
* Feat: add google types
* Feat: add server types
* Feat: transformed code into TypeScript
This allows for users to pass their server functions as a type parameter
to the server class. This can be done by passing the tyepof an object
containing the server functions (their form when importing them from an
index).
import Server from 'gas-client';
import * as serverFunctions from '../server';
const { serverFunctions } = new Server<typeof serverFunctions>();
* Refactor: check env with optional chaining
* Refactor: make other properties private
Prevents users from reassigning serverFunctions and allows config to be
reused in other methods.
* Refactor: change checkAllowList parameter order
Allows the use of optional parameter and creates removal of undefined
type
* Refactor: move constants to specific files
* Refactor: create promisify function
* Refactor: create shouldSetupProxy function
* Refactor: split constructor code in methods
* Refactor: merge multiple return checks
* Build: Remove Babel
Suggestion: Since it's using TypeScript, there's not much use for Babel.
* Refactor: use error message instead of toString
* Refactor: don't require process.env to setup proxy
Suggestion/request: I am using this in an Angular App and the compiler
doesn't pass the node variables to the file, which results in skipping
the proxy setup, since process is not defined. Webpack does provide a
way to mock or even actually pass the variables to the file, but I
couldn't think of some situation where both the error messages and the
production env would need to be checked. I think that just checking the
message would suffice and allow for more flexible setups.
* Style: change class name
* Feature: export ServerFunctions type
From all the types, I think exporting this would be useful for cases
where users declare a typed serverFunctions variable.
import {GasClient, ServerFunctions} from 'gas-client';
const {serverFunctions}: ServerFunctions = new GasClient();
* Docs: update readme
* Docs: fix typos
* Build: remove src from package files
* Style: fix acronym case
* Refactor: change try-catch block to if-else
Use GAS environment to decide if it will setup promises or a proxy.
Allows for unpredicted errors to be thrown so we'll know if something
is broken.
* Style: change type names
* Refactor: move function setup to dedicated classes
* Style: change prettier print width to 120
Since the project is using TypeScript, having 80 print width results in
some very strange line breaks
* Fix: correctly scope window interfaces
* Feature: create dev server types to export
* Docs: setup custom repository and registry
* Style: standardize imports and exports
* Style: remove comments and JSDocs
* Fix: change global types to exports
enuchi
added a commit
that referenced
this pull request
Jul 11, 2021
* Feature/typescript support (#3) * Build: setup TypeScript * Feat: add google types * Feat: add server types * Feat: transformed code into TypeScript This allows for users to pass their server functions as a type parameter to the server class. This can be done by passing the tyepof an object containing the server functions (their form when importing them from an index). import Server from 'gas-client'; import * as serverFunctions from '../server'; const { serverFunctions } = new Server<typeof serverFunctions>(); * Refactor: check env with optional chaining * Refactor: make other properties private Prevents users from reassigning serverFunctions and allows config to be reused in other methods. * Refactor: change checkAllowList parameter order Allows the use of optional parameter and creates removal of undefined type * Refactor: move constants to specific files * Refactor: create promisify function * Refactor: create shouldSetupProxy function * Refactor: split constructor code in methods * Refactor: merge multiple return checks * Build: Remove Babel Suggestion: Since it's using TypeScript, there's not much use for Babel. * Refactor: use error message instead of toString * Refactor: don't require process.env to setup proxy Suggestion/request: I am using this in an Angular App and the compiler doesn't pass the node variables to the file, which results in skipping the proxy setup, since process is not defined. Webpack does provide a way to mock or even actually pass the variables to the file, but I couldn't think of some situation where both the error messages and the production env would need to be checked. I think that just checking the message would suffice and allow for more flexible setups. * Style: change class name * Feature: export ServerFunctions type From all the types, I think exporting this would be useful for cases where users declare a typed serverFunctions variable. import {GasClient, ServerFunctions} from 'gas-client'; const {serverFunctions}: ServerFunctions = new GasClient(); * Docs: update readme * Docs: fix typos * Build: remove src from package files * Style: fix acronym case * Refactor: change try-catch block to if-else Use GAS environment to decide if it will setup promises or a proxy. Allows for unpredicted errors to be thrown so we'll know if something is broken. * Style: change type names * Refactor: move function setup to dedicated classes * Style: change prettier print width to 120 Since the project is using TypeScript, having 80 print width results in some very strange line breaks * Fix: correctly scope window interfaces * Feature: create dev server types to export * Docs: setup custom repository and registry * Style: standardize imports and exports * Style: remove comments and JSDocs * Fix: change global types to exports * update test files and process * Feature/typescript support (#3) * Build: setup TypeScript * Feat: add google types * Feat: add server types * Feat: transformed code into TypeScript This allows for users to pass their server functions as a type parameter to the server class. This can be done by passing the tyepof an object containing the server functions (their form when importing them from an index). import Server from 'gas-client'; import * as serverFunctions from '../server'; const { serverFunctions } = new Server<typeof serverFunctions>(); * Refactor: check env with optional chaining * Refactor: make other properties private Prevents users from reassigning serverFunctions and allows config to be reused in other methods. * Refactor: change checkAllowList parameter order Allows the use of optional parameter and creates removal of undefined type * Refactor: move constants to specific files * Refactor: create promisify function * Refactor: create shouldSetupProxy function * Refactor: split constructor code in methods * Refactor: merge multiple return checks * Build: Remove Babel Suggestion: Since it's using TypeScript, there's not much use for Babel. * Refactor: use error message instead of toString * Refactor: don't require process.env to setup proxy Suggestion/request: I am using this in an Angular App and the compiler doesn't pass the node variables to the file, which results in skipping the proxy setup, since process is not defined. Webpack does provide a way to mock or even actually pass the variables to the file, but I couldn't think of some situation where both the error messages and the production env would need to be checked. I think that just checking the message would suffice and allow for more flexible setups. * Style: change class name * Feature: export ServerFunctions type From all the types, I think exporting this would be useful for cases where users declare a typed serverFunctions variable. import {GasClient, ServerFunctions} from 'gas-client'; const {serverFunctions}: ServerFunctions = new GasClient(); * Docs: update readme * Docs: fix typos * Build: remove src from package files * Style: fix acronym case * Refactor: change try-catch block to if-else Use GAS environment to decide if it will setup promises or a proxy. Allows for unpredicted errors to be thrown so we'll know if something is broken. * Style: change type names * Refactor: move function setup to dedicated classes * Style: change prettier print width to 120 Since the project is using TypeScript, having 80 print width results in some very strange line breaks * Fix: correctly scope window interfaces * Feature: create dev server types to export * Docs: setup custom repository and registry * Style: standardize imports and exports * Style: remove comments and JSDocs * Fix: change global types to exports * Update package.json * revert to named exports * default targetOrigin to '*' * Update README.md * update readme * add src to package * update tests to support '*' targetOrigin * v1.0.0 * update path import * update tests for Boolean(google?.script?.run) * reduce required test coverage to account for typescript compilation branch errors * update package to build esm and run tests against src * remove used ts-node * bump coverage threshold back to 100% Co-authored-by: Guilherme Tod <guilhermetod@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In addition to #2
Apart from some very minor fixes and the readme update, I think that exporting the ServerFunctions type would be useful since I think most people will use the serverFunctions method directly, so they might have to type the variable they assign it to.
I also changed the class name to GasClient (just to match the project name and remove the double server in Server.serverFunctions) and since we're exporting multiple things, I removed the default export. I did not planned to PR this, I was only using it to another private project I have, so it's up to you if you want to keep it like that since I know it's out of your usual pattern.