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.

Sign up for GitHub

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

instanceOf: disable dev instanceOf checks if NODE_ENV not set #4188

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

instanceOf: disable dev instanceOf checks if NODE_ENV not set #4188

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b1daf03
instanceOf: disable dev instanceOf checks if NODE_ENV not set
yaacovCR Sep 9, 2024
9654570
apply review suggestions
yaacovCR Sep 10, 2024
079409e
fix coverage
yaacovCR Sep 10, 2024
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
167 changes: 167 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"test": "npm run lint && npm run check && npm run testonly:cover && npm run prettier:check && npm run check:spelling && npm run check:integrations",
"lint": "eslint --cache --max-warnings 0 --rulesdir resources/eslint-internal-rules/ .",
"check": "tsc --pretty",
"testonly": "mocha --full-trace src/**/__tests__/**/*-test.ts",
"testonly": "dotenvx run --quiet --env NODE_ENV=development -- mocha --full-trace src/**/__tests__/**/*-test.ts",
yaacovCR marked this conversation as resolved.
Show resolved Hide resolved
"testonly:cover": "c8 npm run testonly",
"testonly:watch": "npm run testonly -- --watch",
"prettier": "prettier --cache --cache-strategy metadata --write --list-different .",
Expand All @@ -56,6 +56,7 @@
"devDependencies": {
"@docusaurus/core": "3.5.2",
"@docusaurus/preset-classic": "3.5.2",
"@dotenvx/dotenvx": "^1.14.0",
"@mdx-js/react": "3.0.1",
"@svgr/webpack": "8.1.0",
"@types/chai": "4.3.19",
Expand Down
16 changes: 8 additions & 8 deletions src/jsutils/instanceOf.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { inspect } from './inspect.js';

/* c8 ignore next 3 */
const isProduction =
const isDevelopment =
globalThis.process != null &&
// eslint-disable-next-line no-undef
process.env.NODE_ENV === 'production';
process.env.NODE_ENV === 'development';
Copy link
Member

Choose a reason for hiding this comment

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

test and development should both invoke the non-production behavior.

Copy link
Member

Choose a reason for hiding this comment

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

To minimize the size of this PR, I'd personally rewrite this to just:

const isProduction = process.env.NODE_ENV !== 'development' && process.env.NODE_ENV !== 'test';

Then the lower lines in this PR shouldn't be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

9654570

so this is now:

const isProduction = globalThis.process == null || (process.env.NODE_ENV !== 'development' && process.env.NODE_ENV !== 'test');

retaining the check to see whether process is defined globally

Copy link
Contributor Author

@yaacovCR yaacovCR Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

JoviDeCroock/graphql-minifier-experiments#1

I have not gone through all the bundlers, but I think that flipping the default would require us changing our instructions about what to set globalThis.process to, now we would have to set to null

https://github.com/graphql/graphql-js/blob/16.x.x/website/docs/tutorials/going-to-production.md

and if I remember correctly, the reason that is in at all is to support cloudflare.

So I am not sure if I want to move forward with this PR actually.... as if we have a better long-term solution, we should move to that without this intermediate stage......?

@JoviDeCroock @benjie @phryneas

Copy link

Choose a reason for hiding this comment

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

Long-term solution: 3 versions of this file - one with the development export condition, without any of this logic, one with the production export condition without any of this, and one default that falls back to this (or a similar) solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phryneas I closed #4221 because once we use an imports map, it becomes tricky to use graphql in browsers in a bundler-free manner. I think unfortunately using the exports map and publishing instanceOf might have the same problem.

Copy link

@phryneas phryneas Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@yaacovCR What problems are you seeing?
Generally, I'd say that unless we publish only a single file, users will have to adopt import maps, but that's the future of ESM on the web anyways.

PS: see https://github.com/apollographql/apollo-client/blob/main/integration-tests/browser-esm/html/jspm-prepared.html for an example

Copy link

Choose a reason for hiding this comment

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

(That said, if we were to continue with #4221, we would need to ship a rolled-up browser entry point that didn't reference imports - maybe with a separate development and production build.
To be honest, I don't really think we'd need sub-entrypoint browser builds, people who run without a bundler probably don't care about saving bundlesize anyways.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not sure if we want to require the use of an import map but I would definitely defer to @JoviDeCroock on that.

Copy link
Member

@JoviDeCroock JoviDeCroock Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@phryneas Import maps are still being adopted and imho aren't in a well enough state yet for a reference implementation based on a standard to adopt nor is the spec far along enough for it. My two cents here being that either we remove instanceof all together and replace it with a branded symbol/types only approach or we keep the status quo but default to production instead.


/**
* A replacement for instanceof which includes an error warning when multi-realm
Expand All @@ -13,13 +13,8 @@ const isProduction =
* See: https://webpack.js.org/guides/production/
*/
export const instanceOf: (value: unknown, constructor: Constructor) => boolean =
/* c8 ignore next 6 */
// FIXME: https://github.com/graphql/graphql-js/issues/2317
isProduction
isDevelopment
? function instanceOf(value: unknown, constructor: Constructor): boolean {
return value instanceof constructor;
}
: function instanceOf(value: unknown, constructor: Constructor): boolean {
if (value instanceof constructor) {
return true;
}
Expand Down Expand Up @@ -50,6 +45,11 @@ spurious results.`,
}
}
return false;
}
: /* c8 ignore next 4 */
// FIXME: https://github.com/graphql/graphql-js/issues/2317
function instanceOf(value: unknown, constructor: Constructor): boolean {
return value instanceof constructor;
};

interface Constructor extends Function {
Expand Down
Loading