Skip to content

chore(various): Fix comments and docstrings #5651

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

Merged
merged 25 commits into from
Aug 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
542d94f
wordsmith utils package readme
lobsterkatie May 12, 2022
b3b5da4
add comment about root dir to central jest config
lobsterkatie May 12, 2022
6888f01
add link to eslint config docs to main eslint config file
lobsterkatie May 12, 2022
bee242c
add comment to GHA linting job
lobsterkatie May 12, 2022
f0f3ea1
add link to commonjs rollup plugin to bundle config
lobsterkatie May 12, 2022
373836e
fix comment and docstring in rollup bundle plugins
lobsterkatie May 12, 2022
709886f
fix typo in syncpromise test title
lobsterkatie May 12, 2022
9f392a4
fix docstring on scope method
lobsterkatie May 12, 2022
088d972
fix docstring on uncaught exception handler in node
lobsterkatie May 12, 2022
88f2ae5
fix docstring on `getIntegration`
lobsterkatie May 12, 2022
1167a3b
fix docstrings in `normalize.ts` and normalization tests
lobsterkatie May 12, 2022
ef1017c
wordsmith gatsby browser file
lobsterkatie May 12, 2022
672d586
fix comments in nextjs types file
lobsterkatie May 12, 2022
023eede
add comments in launch.json
lobsterkatie May 12, 2022
1e40494
clarify variable name, add docstring for `isBuiltin`
lobsterkatie May 12, 2022
7d69113
fix comment in nextjs integration test runner script
lobsterkatie May 12, 2022
6010cee
clarify wording of comment in `tracing/index.bundle.ts`
lobsterkatie May 12, 2022
c043043
code-ify changelog entry for clarity
lobsterkatie May 12, 2022
9362548
remove outdated todo in nextjs webpack config code
lobsterkatie May 12, 2022
c58a739
add filename to error message in ts version check
lobsterkatie May 12, 2022
50d9032
add links to hooks docs to npm rollup plugins
lobsterkatie May 17, 2022
ed8550c
fix typo in aws lambda comment
lobsterkatie May 19, 2022
41991a1
add TODO re: v8 to hub test
lobsterkatie Aug 30, 2022
8fe1a80
add blank lines between node client tests
lobsterkatie Aug 30, 2022
d240024
fix error messages re: full buffers
lobsterkatie Aug 30, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Note: All paths are relative to the directory in which eslint is being run, rather than the directory where this file
// lives

// ESLint config docs: https://eslint.org/docs/user-guide/configuring/

module.exports = {
root: true,
env: {
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,8 @@ jobs:

job_lint:
name: Lint
# Even though the linter only checks source code, not built code, it needs the built code in order check that all
# inter-package dependencies resolve cleanly.
needs: [job_get_metadata, job_build]
timeout-minutes: 10
runs-on: ubuntu-latest
Expand Down
4 changes: 4 additions & 0 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@
// Use IntelliSense to learn about possible Node.js debug attributes.
// Hover to view descriptions of existing attributes.
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
// For available variables, visit: https://code.visualstudio.com/docs/editor/variables-reference
"version": "0.2.0",
"inputs": [
{
// Get the name of the package containing the file in the active tab.
"id": "getPackageName",
"type": "command",
"command": "shellCommand.execute",
"args": {
// Get the current file's absolute path, chop off everything up to and including the repo's `packages`
// directory, then split on `/` and take the first entry
"command": "echo '${file}' | sed s/'.*sentry-javascript\\/packages\\/'// | grep --extended-regexp --only-matching --max-count 1 '[^\\/]+' | head -1",
"cwd": "${workspaceFolder}",
// normally `input` commands bring up a selector for the user, but given that there should only be one
Expand Down
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ Work in this release contributed by @7inspire, @jaeseokk, and @rchl. Thank you f
This patch contains a breaking change for anyone setting the undocumented `rethrowAfterCapture` option for `@sentry/serverless`'s AWS wrapper to `false`, as its functionality has been removed. For backwards compatibility with anyone setting it to `true` (which is also the default), the option remains in the `WrapperOptions` type for now. It will be removed in the next major release, though, so we recommend removing it from your code.

- ref(serverless): Remove `rethrowAfterCapture` use in AWS lambda wrapper (#4448)
- fix(utils): Remove dom is casting (#4451)
- fix(utils): Remove dom `is` casting (#4451)

## 6.17.1

Expand Down
1 change: 1 addition & 0 deletions jest/jest.config.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
module.exports = {
// this is the package root, even when tests are being run at the repo level
rootDir: process.cwd(),
collectCoverage: true,
transform: {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/transports/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export function createTransport(
result => result,
error => {
if (error instanceof SentryError) {
__DEBUG_BUILD__ && logger.error('Skipped sending event due to full buffer');
__DEBUG_BUILD__ && logger.error('Skipped sending event because buffer is full.');
recordEnvelopeLoss('queue_overflow');
return resolvedSyncPromise();
} else {
Expand Down
6 changes: 3 additions & 3 deletions packages/gatsby/gatsby-browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ exports.onClientEntry = function (_, pluginParams) {
if (areOptionsDefined) {
console.warn(
'Sentry Logger [Warn]: The SDK was initialized in the Sentry config file, but options were found in the Gatsby config. ' +
'These have been ignored, merge them to the Sentry config if you want to use them.\n' +
'Learn more about the Gatsby SDK on https://docs.sentry.io/platforms/javascript/guides/gatsby/',
'These have been ignored. Merge them to the Sentry config if you want to use them.\n' +
'Learn more about the Gatsby SDK in https://docs.sentry.io/platforms/javascript/guides/gatsby/.',
);
}
return;
Expand All @@ -19,7 +19,7 @@ exports.onClientEntry = function (_, pluginParams) {
if (!areOptionsDefined) {
console.error(
'Sentry Logger [Error]: No config for the Gatsby SDK was found.\n' +
'Learn how to configure it on https://docs.sentry.io/platforms/javascript/guides/gatsby/',
'Learn how to configure it in https://docs.sentry.io/platforms/javascript/guides/gatsby/.',
);
return;
}
Expand Down
6 changes: 3 additions & 3 deletions packages/gatsby/test/gatsby-browser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ describe('onClientEntry', () => {
// eslint-disable-next-line no-console
expect((console.warn as jest.Mock).mock.calls[0]).toMatchInlineSnapshot(`
Array [
"Sentry Logger [Warn]: The SDK was initialized in the Sentry config file, but options were found in the Gatsby config. These have been ignored, merge them to the Sentry config if you want to use them.
Learn more about the Gatsby SDK on https://docs.sentry.io/platforms/javascript/guides/gatsby/",
"Sentry Logger [Warn]: The SDK was initialized in the Sentry config file, but options were found in the Gatsby config. These have been ignored. Merge them to the Sentry config if you want to use them.
Learn more about the Gatsby SDK in https://docs.sentry.io/platforms/javascript/guides/gatsby/.",
]
`);
// eslint-disable-next-line no-console
Expand All @@ -98,7 +98,7 @@ describe('onClientEntry', () => {
expect((console.error as jest.Mock).mock.calls[0]).toMatchInlineSnapshot(`
Array [
"Sentry Logger [Error]: No config for the Gatsby SDK was found.
Learn how to configure it on https://docs.sentry.io/platforms/javascript/guides/gatsby/",
Learn how to configure it in https://docs.sentry.io/platforms/javascript/guides/gatsby/.",
]
`);
});
Expand Down
7 changes: 3 additions & 4 deletions packages/hub/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -441,11 +441,10 @@ export class Scope implements ScopeInterface {
}

/**
* Applies the current context and fingerprint to the event.
* Note that breadcrumbs will be added by the client.
* Also if the event has already breadcrumbs on it, we do not merge them.
* Applies data from the scope to the event and runs all event processors on it.
*
* @param event Event
* @param hint May contain additional information about the original exception.
* @param hint Object containing additional information about the original exception, for use by the event processors.
* @hidden
*/
public applyToEvent(event: Event, hint: EventHint = {}): PromiseLike<Event | null> {
Expand Down
1 change: 1 addition & 0 deletions packages/hub/test/exports.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ describe('Top Level API', () => {
});

// NOTE: We left custom level as 2nd argument to not break the API. Should be removed and unified in v6.
// TODO: Before we release v8, check if this is still a thing
test('Message with custom level', () => {
const client: any = { captureMessage: jest.fn(async () => Promise.resolve()) };
getCurrentHub().withScope(() => {
Expand Down
1 change: 1 addition & 0 deletions packages/integrations/rollup.bundle.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const baseBundleConfig = makeBaseBundleConfig({
});

// TODO We only need `commonjs` for localforage (used in the offline plugin). Once that's fixed, this can come out.
// CommonJS plugin docs: https://github.com/rollup/plugins/tree/master/packages/commonjs
baseBundleConfig.plugins = insertAt(baseBundleConfig.plugins, -2, commonjs());

// this makes non-minified, minified, and minified-with-debug-logging versions of each bundle
Expand Down
18 changes: 10 additions & 8 deletions packages/nextjs/src/config/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ export type NextConfigFunctionWithSentry = (
) => NextConfigObjectWithSentry;

export type NextConfigObject = {
// custom webpack options
// Custom webpack options
webpack?: WebpackConfigFunction;
// whether to build serverless functions for all pages, not just API routes
// Whether to build serverless functions for all pages, not just API routes. Removed in nextjs 12+.
target?: 'server' | 'experimental-serverless-trace';
// the output directory for the built app (defaults to ".next")
// The output directory for the built app (defaults to ".next")
distDir?: string;
// the root at which the nextjs app will be served (defaults to "/")
// The root at which the nextjs app will be served (defaults to "/")
basePath?: string;
// config which will be available at runtime
// Config which will be available at runtime
publicRuntimeConfig?: { [key: string]: unknown };
};

Expand All @@ -39,6 +39,9 @@ export type UserSentryOptions = {
// the plugin to be enabled, even in situations where it's not recommended.
disableServerWebpackPlugin?: boolean;
disableClientWebpackPlugin?: boolean;

// Use `hidden-source-map` for webpack `devtool` option, which strips the `sourceMappingURL` from the bottom of built
// JS files
hideSourceMaps?: boolean;

// Force webpack to apply the same transpilation rules to the SDK code as apply to user code. Helpful when targeting
Expand All @@ -64,9 +67,8 @@ export type NextConfigFunction = (phase: string, defaults: { defaultConfig: Next
* Webpack config
*/

// the format for providing custom webpack config in your nextjs options
// The two possible formats for providing custom webpack config in `next.config.js`
export type WebpackConfigFunction = (config: WebpackConfigObject, options: BuildContext) => WebpackConfigObject;

export type WebpackConfigObject = {
devtool?: string;
plugins?: Array<WebpackPluginInstance | SentryWebpackPlugin>;
Expand All @@ -81,7 +83,7 @@ export type WebpackConfigObject = {
rules: Array<WebpackModuleRule>;
};
} & {
// other webpack options
// Other webpack options
[key: string]: unknown;
};

Expand Down
2 changes: 1 addition & 1 deletion packages/nextjs/src/config/webpack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export { SentryWebpackPlugin };
* Sets:
* - `devtool`, to ensure high-quality sourcemaps are generated
* - `entry`, to include user's sentry config files (where `Sentry.init` is called) in the build
* - `plugins`, to add SentryWebpackPlugin (TODO: optional)
* - `plugins`, to add SentryWebpackPlugin
*
* @param userNextConfig The user's existing nextjs config, as passed to `withSentryConfig`
* @param userSentryWebpackPluginOptions The user's SentryWebpackPlugin config, as passed to `withSentryConfig`
Expand Down
2 changes: 1 addition & 1 deletion packages/nextjs/test/run-integration-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ for NEXTJS_VERSION in 10 11 12; do

echo "[nextjs@$NEXTJS_VERSION] Installing dependencies..."
# set the desired version of next long enough to run yarn, and then restore the old version (doing the restoration now
# rather than during overall cleanup lets us look for "latest" in both loops)
# rather than during overall cleanup lets us look for "latest" in every loop)
cp package.json package.json.bak
if [[ $(uname) == "Darwin" ]]; then
sed -i "" /"next.*latest"/s/latest/"${NEXTJS_VERSION}.x"/ package.json
Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/integrations/onuncaughtexception.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { logAndExitProcess } from './utils/errorhandling';

type OnFatalErrorHandler = (firstError: Error, secondError?: Error) => void;

/** Global Promise Rejection handler */
/** Global Exception handler */
export class OnUncaughtException implements Integration {
/**
* @inheritDoc
Expand Down
4 changes: 4 additions & 0 deletions packages/node/test/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('NodeClient', () => {
const requestSession = scope.getRequestSession();
expect(requestSession!.status).toEqual('ok');
});

test('when autoSessionTracking is disabled -> requestStatus should not be set', () => {
const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN, autoSessionTracking: false, release: '1.4' });
client = new NodeClient(options);
Expand All @@ -42,6 +43,7 @@ describe('NodeClient', () => {
const requestSession = scope.getRequestSession();
expect(requestSession!.status).toEqual('ok');
});

test('when autoSessionTracking is enabled + requestSession status is Crashed -> requestStatus should not be overridden', () => {
const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' });
client = new NodeClient(options);
Expand All @@ -57,6 +59,7 @@ describe('NodeClient', () => {
const requestSession = scope.getRequestSession();
expect(requestSession!.status).toEqual('crashed');
});

test('when autoSessionTracking is enabled + error occurs within request bounds -> requestStatus should be set to Errored', () => {
const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' });
client = new NodeClient(options);
Expand All @@ -72,6 +75,7 @@ describe('NodeClient', () => {
const requestSession = scope.getRequestSession();
expect(requestSession!.status).toEqual('errored');
});

test('when autoSessionTracking is enabled + error occurs outside of request bounds -> requestStatus should not be set to Errored', () => {
const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN, autoSessionTracking: true, release: '1.4' });
client = new NodeClient(options);
Expand Down
2 changes: 1 addition & 1 deletion packages/serverless/src/awslambda.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ export function wrapHandler<TEvent, TResult>(
};
let timeoutWarningTimer: NodeJS.Timeout;

// AWSLambda is like Express. It makes a distinction about handlers based on it's last argument
// AWSLambda is like Express. It makes a distinction about handlers based on its last argument
// async (event) => async handler
// async (event, context) => async handler
// (event, context, callback) => sync handler
Expand Down
8 changes: 4 additions & 4 deletions packages/tracing/src/index.bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@ const INTEGRATIONS = {
};

export { INTEGRATIONS as Integrations };
// Though in this case exporting this separately in addition to exporting it as part of `Sentry.Integrations` doesn't
// gain us any bundle size advantage (we're making the bundle here, not the user, and we can't leave anything out of
// ours), it does bring the API for using the integration in line with that recommended for users bundling Sentry
// themselves.
// Though in this case exporting `BrowserTracing` separately (in addition to exporting it as part of
// `Sentry.Integrations`) doesn't gain us any bundle size advantage (we're making the bundle here, not the user, and we
// can't leave anything out of ours), it does bring the API for using the integration in line with that recommended for
// users bundling Sentry themselves.
export { BrowserTracing };

// We are patching the global object with our hub extension methods
Expand Down
2 changes: 1 addition & 1 deletion packages/types/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export interface Client<O extends ClientOptions = ClientOptions> {
*/
flush(timeout?: number): PromiseLike<boolean>;

/** Returns an array of installed integrations on the client. */
/** Returns the client's instance of the given integration class, it any. */
getIntegration<T extends Integration>(integration: IntegrationClass<T>): T | null;

/** This is an internal function to setup all integrations that should run on the client */
Expand Down
6 changes: 3 additions & 3 deletions packages/utils/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@

## General

Common utilities used by the Sentry JavaScript SDKs. **Warning, only submodule imports are allowed here.** Also note,
this function shouldn't be used externally, we will break them from time to time. This package is not part of our public
API contract, we use them only internally.
Common utilities used by the Sentry JavaScript SDKs.

Note: This package is only meant to be used internally, and as such is not part of our public API contract and does not follow semver.
12 changes: 9 additions & 3 deletions packages/utils/src/is.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,15 @@ export function isError(wat: unknown): wat is Error {
return isInstanceOf(wat, Error);
}
}

function isBuiltin(wat: unknown, ty: string): boolean {
return objectToString.call(wat) === `[object ${ty}]`;
/**
* Checks whether given value is an instance of the given built-in class.
*
* @param wat The value to be checked
* @param className
* @returns A boolean representing the result.
*/
function isBuiltin(wat: unknown, className: string): boolean {
return objectToString.call(wat) === `[object ${className}]`;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/utils/src/normalize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ type ObjOrArray<T> = { [key: string]: T };
* @param input The object to be normalized.
* @param depth The max depth to which to normalize the object. (Anything deeper stringified whole.)
* @param maxProperties The max number of elements or properties to be included in any single array or
* object in the normallized output..
* object in the normallized output.
* @returns A normalized version of the object, or `"**non-serializable**"` if any errors are thrown during normalization.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down
2 changes: 1 addition & 1 deletion packages/utils/src/promisebuffer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export function makePromiseBuffer<T>(limit?: number): PromiseBuffer<T> {
*/
function add(taskProducer: () => PromiseLike<T>): PromiseLike<T> {
if (!isReady()) {
return rejectedSyncPromise(new SentryError('Not adding Promise due to buffer limit reached.'));
return rejectedSyncPromise(new SentryError('Not adding Promise because buffer limit was reached.'));
}

// start the task and add its promise to the queue
Expand Down
8 changes: 4 additions & 4 deletions packages/utils/test/normalize.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ describe('normalize()', () => {
});
});

describe('dont mutate and skip non-enumerables', () => {
describe("doesn't mutate the given object and skips non-enumerables", () => {
test('simple object', () => {
const circular = {
foo: 1,
Expand Down Expand Up @@ -311,7 +311,7 @@ describe('normalize()', () => {
});
});

describe('changes unserializeable/global values/classes to its string representation', () => {
describe('changes unserializeable/global values/classes to their respective string representations', () => {
test('primitive values', () => {
expect(normalize(undefined)).toEqual('[undefined]');
expect(normalize(NaN)).toEqual('[NaN]');
Expand Down Expand Up @@ -376,7 +376,7 @@ describe('normalize()', () => {
});
});

test('known Classes like Reacts SyntheticEvents', () => {
test("known classes like React's `SyntheticEvent`", () => {
const obj = {
foo: {
nativeEvent: 'wat',
Expand Down Expand Up @@ -510,7 +510,7 @@ describe('normalize()', () => {
});
});

test('normalizes value on every iteration of decycle and takes care of things like Reacts SyntheticEvents', () => {
test("normalizes value on every iteration of decycle and takes care of things like React's `SyntheticEvent`", () => {
const obj = {
foo: {
nativeEvent: 'wat',
Expand Down
2 changes: 1 addition & 1 deletion packages/utils/test/syncpromise.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ describe('SyncPromise', () => {
});
});

test('calling the callback immediatly', () => {
test('calling the callback immediately', () => {
expect.assertions(1);

let foo: number = 1;
Expand Down
9 changes: 6 additions & 3 deletions rollup/plugins/bundlePlugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,15 @@ export function makeLicensePlugin(title) {
* Create a plugin to set the value of the `__SENTRY_DEBUG__` magic string.
*
* @param includeDebugging Whether or not the resulting build should include log statements
* @returns An instance of the `replace` plugin to do the replacement of the magic string with `true` or 'false`
* @returns An instance of the `@rollup/plugin-replace` plugin to do the replacement of the magic string with `true` or
* 'false`
*/
export function makeIsDebugBuildPlugin(includeDebugging) {
return replace({
// __DEBUG_BUILD__ and __SENTRY_DEBUG__ are safe to replace in any case, so no checks for assignments necessary
preventAssignment: false,
// TODO `preventAssignment` will default to true in version 5.x of the replace plugin, at which point we can get rid
// of this. (It actually makes no difference in this case whether it's true or false, since we never assign to
// `__SENTRY_DEBUG__`, but if we don't give it a value, it will spam with warnings.)
preventAssignment: true,
values: {
// Flags in current package
__DEBUG_BUILD__: includeDebugging,
Expand Down
Loading