Skip to content
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

ref(nextjs): Prework for wrapping data-fetching functions #5508

Merged
merged 4 commits into from Aug 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/nextjs/.eslintrc.js
Expand Up @@ -11,4 +11,12 @@ module.exports = {
rules: {
'@sentry-internal/sdk/no-async-await': 'off',
},
overrides: [
{
files: ['scripts/**/*.ts'],
parserOptions: {
project: ['../../tsconfig.dev.json'],
},
},
],
};
4 changes: 2 additions & 2 deletions packages/nextjs/package.json
Expand Up @@ -45,11 +45,11 @@
"scripts": {
"build": "run-p build:rollup build:types",
"build:dev": "run-s build",
"build:rollup": "rollup -c rollup.npm.config.js",
"build:rollup": "ts-node scripts/buildRollup.ts",
"build:types": "tsc -p tsconfig.types.json",
"build:watch": "run-p build:rollup:watch build:types:watch",
"build:dev:watch": "run-s build:watch",
"build:rollup:watch": "rollup -c rollup.npm.config.js --watch",
"build:rollup:watch": "nodemon --ext ts --watch src scripts/buildRollup.ts",
"build:types:watch": "tsc -p tsconfig.types.json --watch",
"build:npm": "ts-node ../../scripts/prepack.ts && npm pack ./build",
"circularDepCheck": "madge --circular src/index.client.ts && madge --circular --exclude 'config/types\\.ts' src/index.server.ts # see https://github.com/pahen/madge/issues/306",
Expand Down
8 changes: 4 additions & 4 deletions packages/nextjs/rollup.npm.config.js
Expand Up @@ -14,12 +14,12 @@ export default [
),
...makeNPMConfigVariants(
makeBaseNPMConfig({
entrypoints: ['src/config/prefixLoaderTemplate.ts'],
entrypoints: ['src/config/templates/prefixLoaderTemplate.ts'],

packageSpecificConfig: {
output: {
// preserve the original file structure (i.e., so that everything is still relative to `src`)
entryFileNames: 'config/[name].js',
entryFileNames: 'config/templates/[name].js',

// this is going to be add-on code, so it doesn't need the trappings of a full module (and in fact actively
// shouldn't have them, lest they muck with the module to which we're adding it)
Expand All @@ -31,15 +31,15 @@ export default [
),
...makeNPMConfigVariants(
makeBaseNPMConfig({
entrypoints: ['src/config/prefixLoader.ts'],
entrypoints: ['src/config/loaders/prefixLoader.ts'],

packageSpecificConfig: {
output: {
// make it so Rollup calms down about the fact that we're doing `export { loader as default }`
exports: 'default',

// preserve the original file structure (i.e., so that everything is still relative to `src`)
entryFileNames: 'config/[name].js',
entryFileNames: 'config/loaders/[name].js',
},
},
}),
Expand Down
24 changes: 24 additions & 0 deletions packages/nextjs/scripts/buildRollup.ts
@@ -0,0 +1,24 @@
import * as childProcess from 'child_process';
import * as fs from 'fs';
import * as path from 'path';

/**
* Run the given shell command, piping the shell process's `stdin`, `stdout`, and `stderr` to that of the current
* process. Returns contents of `stdout`.
*/
function run(cmd: string, options?: childProcess.ExecSyncOptions): string | Buffer {
return childProcess.execSync(cmd, { stdio: 'inherit', ...options });
}

run('yarn rollup -c rollup.npm.config.js');

// Regardless of whether nextjs is using the CJS or ESM version of our SDK, we want the code from our templates to be in
// ESM (since we'll be adding it onto page files which are themselves written in ESM), so copy the ESM versions of the
// templates over into the CJS build directory. (Building only the ESM version and sticking it in both locations is
// something which in theory Rollup could do, but it would mean refactoring our Rollup helper functions, which isn't
// worth it just for this.)
Comment on lines +15 to +19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the kind of technical debt we will never get rid of. Let's leave it but take the learning that DRY has it's downsides in regards to rollup configs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting - not sure I'd call this tech debt. This is a weird one-off, and I feel like quite often in those cases the answer isn't "make your thing (whatever it is) support the uncommon edge case" it's "find a workaround," which to me is exactly what this is, no?

const cjsTemplateDir = 'build/cjs/config/templates/';
const esmTemplateDir = 'build/esm/config/templates/';
fs.readdirSync(esmTemplateDir).forEach(templateFile =>
fs.copyFileSync(path.join(esmTemplateDir, templateFile), path.join(cjsTemplateDir, templateFile)),
);
@@ -1,27 +1,22 @@
import * as fs from 'fs';
import * as path from 'path';

import { LoaderThis } from './types';

type LoaderOptions = {
distDir: string;
};
// TODO Use real webpack types
type LoaderThis = {
// Webpack 4
query?: LoaderOptions;
// Webpack 5
getOptions?: () => LoaderOptions;
addDependency: (filepath: string) => void;
};

/**
* Inject templated code into the beginning of a module.
*/
function prefixLoader(this: LoaderThis, userCode: string): string {
function prefixLoader(this: LoaderThis<LoaderOptions>, userCode: string): string {
// We know one or the other will be defined, depending on the version of webpack being used
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const { distDir } = this.getOptions ? this.getOptions() : this.query!;

const templatePath = path.resolve(__dirname, 'prefixLoaderTemplate.js');
const templatePath = path.resolve(__dirname, '../templates/prefixLoaderTemplate.js');
// make sure the template is included when runing `webpack watch`
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
this.addDependency(templatePath);

// Fill in the placeholder
Expand Down
10 changes: 10 additions & 0 deletions packages/nextjs/src/config/loaders/types.ts
@@ -0,0 +1,10 @@
// TODO Use real webpack types
export type LoaderThis<Options> = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally optional but we could type this as { addDependency: blah } & ({ query?: blah } | { getOptions?: blah }) so we have the "either-or" nature of this type baked in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Will do.

UPDATE: Turns out TS can't handle it, at least not gracefully. Doing that make lines like

const { distDir } = this.getOptions ? this.getOptions() : this.query

error out with

Property 'getOptions' does not exist on type 'LoaderThis<LoaderOptions>'.
  Property 'getOptions' does not exist on type '{ query: LoaderOptions; } & { addDependency: (filepath: string) => void; }'

Annoying that it won't let you even check to see if something is there, but not sure it's worth trying to convince it otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right should have tried it out - my bad. I think you need to do the check something like 'getOptions' in this ? ... : .... But doesn't really matter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but then you run into microsoft/TypeScript#21732. (Specifically - switching to 'getOptions' in this ? this.getOptions() : this.query results in Cannot invoke an object which is possibly 'undefined'. (about this.getOptions()), while still leaving you with the error above with respect to this.query.)

TS - can't live with it, can't live without it.

// Loader options in Webpack 4
query?: Options;
// Loader options in Webpack 5
getOptions?: () => Options;

// Function to add outside file used by loader to `watch` process
addDependency: (filepath: string) => void;
};
@@ -1,5 +1,6 @@
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
(global as any).__rewriteFramesDistDir__ = '__DIST_DIR__';

// We need this to make this file an ESM module, which TS requires when using `isolatedModules`.
// We need this to make this file an ESM module, which TS requires when using `isolatedModules`, but it doesn't affect
// the end result - Rollup recognizes that it's a no-op and doesn't include it when building our code.
export {};
2 changes: 1 addition & 1 deletion packages/nextjs/src/config/webpack.ts
Expand Up @@ -65,7 +65,7 @@ export function constructWebpackConfigFunction(
// Support non-default output directories by making the output path (easy to get here at build-time)
// available to the server SDK's default `RewriteFrames` instance (which needs it at runtime), by
// injecting code to attach it to `global`.
loader: path.resolve(__dirname, 'prefixLoader.js'),
loader: path.resolve(__dirname, 'loaders/prefixLoader.js'),
options: {
distDir: userNextConfig.distDir || '.next',
},
Expand Down