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

Jump to bottom

feat: esm support #161

Closed
wants to merge 5 commits into from
Closed

feat: esm support #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3319db8
feat: remove @restart/hooks dependency
n1ru4l Feb 12, 2021
90d2883
fix: add es module export
n1ru4l Feb 12, 2021
5a9295c
chore: use .mjs import
n1ru4l Feb 12, 2021
512af97
chore: please the linter
n1ru4l Feb 15, 2021
90e6d1f
feat: move compiled cjs files to lib/cjs (with commonjs package.json)
n1ru4l May 19, 2021
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
15 changes: 11 additions & 4 deletions package.json
View file
Open in desktop
Expand Up @@ -8,7 +8,15 @@
"react",
"ssr"
],
"main": "lib/index.js",
"main": "./lib/cjs/cjs-relay-hooks.js",
"module": "./lib/es-relay-hooks.mjs",
"exports": {
".": {
"import": "./lib/es-relay-hooks.mjs",
"require": "./lib/cjs/cjs-relay-hooks.js"
},
"./package.json": "./package.json"
},
Comment on lines +13 to +19
Copy link
Member Author

Choose a reason for hiding this comment

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

Starting with Node.js 14 you cannot simply import anything from a package. You can only use the exports declared in the exports map.

E.g. import { fetchResolver } from "relay-hooks/lib/FetchResolver"; would fail. If that should be possible a entry for each file must be declared in the exports map.

Copy link
Member

Choose a reason for hiding this comment

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

hi @n1ru4l,
i tried this package in react-relay-offline and i get this error.

react-relay-offline\node_modules\relay-hooks\lib\QueryFetcher.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import * as areEqual from 'fbjs/lib/areEqual';
    SyntaxError: Cannot use import statement outside a module

Did you mean this?

Copy link
Member Author

@n1ru4l n1ru4l May 19, 2021 •
edited

Choose a reason for hiding this comment

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

If you are referring to this: https://github.com/morrys/react-relay-offline/blob/c12787d3d0c3325389cd4c8d06ea2f3ffdac800a/src/runtime/loadQuery.ts#L3

Then yes, instead this file should be explicitly exposed via the exports map. See https://nodejs.org/api/packages.html#packages_package_entry_points

Is there any specific reason why you don't re-export those files from the entry point (in relay-hooks) and import it from there (in react-relay-offline)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also related to #161 (comment)

"license": "MIT",
"description": "Relay Hooks",
"author": {
Expand All @@ -21,18 +29,17 @@
},
"scripts": {
"clean": "rimraf lib",
"compile": "npm run clean && tsc && npm run build:js && npm run rollup",
"compile": "npm run clean && tsc && tsc --project tsconfig.esm.json && npm run build:js && npm run rollup",
"rollup": "rollup -c",
"build": "npm run compile && npm run test",
"test": "cross-env NODE_ENV=test jest --coverage",
"format": "prettier --write \"src/**/*.{j,t}s*\"",
"format:ci": "prettier --list-different \"src/**/*.{j,t}s*\"",
"eslint": "eslint ./src --ext .js,.jsx,.ts,.tsx",
"prepublishOnly": "npm run build",
"build:js": "babel lib --out-dir lib --extensions \".js,.jsx\""
"build:js": "babel lib/cjs --out-dir lib/cjs --extensions \".js,.jsx\""
},
"dependencies": {
"@restart/hooks": "^0.3.1",
"fbjs": "^3.0.0"
},
"peerDependencies": {
Expand Down
26 changes: 24 additions & 2 deletions rollup.config.js
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ import replace from '@rollup/plugin-replace';
import sourceMaps from 'rollup-plugin-sourcemaps';
import { terser } from 'rollup-plugin-terser';
import typescript from 'rollup-plugin-typescript2';
import { promises as fs } from "fs";
import pkg from './package.json';

const makeExternalPredicate = (externalArr) => {
Expand All @@ -24,15 +25,14 @@ function createConfigInternal({ format, production }) {
return {
input: 'src/index.ts',
output: {
file: 'lib/' + format + '-relay-hooks' + (production ? '.min' : '') + '.js',
file: 'lib/' + (format === "cjs" ? "cjs/" : "") + format + '-relay-hooks' + (production ? '.min' : '') + '.js',
format,
name: 'relay-hooks',
indent: false,
globals: {
'fbjs/lib/areEqual': 'areEqual',
'fbjs/lib/invariant': 'invariant',
'fbjs/lib/warning': 'warning',
'@restart/hooks/useMounted': 'useMounted',
react: 'React',
'relay-runtime': 'relayRuntime',
},
Expand Down Expand Up @@ -83,6 +83,28 @@ function createConfigInternal({ format, production }) {
toplevel: format === 'cjs',
warnings: true,
}),
format === "cjs" && {
name: "writePkgJSON",
writeBundle: async () => {
await fs.writeFile(
"lib/cjs/package.json",
JSON.stringify({
type: "commonjs",
})
);
}
},
format === "es" && {
name: "writePkgJSON",
writeBundle: async () => {
await fs.writeFile(
"lib/package.json",
JSON.stringify({
type: "module",
})
);
}
},
Comment on lines +86 to +107
Copy link
Member Author

Choose a reason for hiding this comment

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

A lib/package.json with the contents {"type":"module"} and also a lib/cjs/package.json with the contents {"type":"commonjs"} is generated.

This tells Node.js to treat the contents of that folder/subfolder as the given file type without having to specify the .cjs or .mjs extension. I did not specify it in the root package.json as it caused issues with babel and jest (jest is not es ready yet).

Copy link
Member Author

@n1ru4l n1ru4l May 19, 2021 •
edited

Choose a reason for hiding this comment

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

I also noticed that the lib folder also includes the "raw" js files besides the bundles. Do we really need those raw files? Why would one want to import those directly instead of using the top-level exports, do you have any use-case in mind? Would it be enough to simply have the typescript definition files and the bundled rollup code?

],
};
}
Expand Down
16 changes: 10 additions & 6 deletions src/useMutation.ts
View file
Open in desktop
@@ -1,4 +1,3 @@
import useMounted from '@restart/hooks/useMounted';
import * as invariant from 'fbjs/lib/invariant';
import * as React from 'react';
import { Environment, MutationParameters, commitMutation } from 'relay-runtime';
Expand Down Expand Up @@ -38,7 +37,13 @@ export function useMutation<T extends MutationParameters>(
error: null,
});

const isMounted = useMounted();
const isMountedRef = React.useRef(true);
React.useEffect(
(): (() => void) => (): void => {
isMountedRef.current = false;
},
[],
);

const relayEnvironment = useRelayEnvironment();
const resolvedEnvironment = environment || relayEnvironment;
Expand Down Expand Up @@ -69,7 +74,7 @@ export function useMutation<T extends MutationParameters>(

invariant(mergedConfig.variables, 'you must specify variables');

if (isMounted()) {
if (isMountedRef.current) {
setState({
loading: true,
data: mergedConfig.optimisticResponse,
Expand All @@ -79,7 +84,7 @@ export function useMutation<T extends MutationParameters>(

return new Promise((resolve, reject) => {
function handleError(error: any): void {
if (isMounted()) {
if (isMountedRef.current) {
setState({
loading: false,
data: null,
Expand All @@ -106,7 +111,7 @@ export function useMutation<T extends MutationParameters>(
return;
}

if (isMounted()) {
if (isMountedRef.current) {
setState({
loading: false,
data: response,
Expand Down Expand Up @@ -134,7 +139,6 @@ export function useMutation<T extends MutationParameters>(
optimisticUpdater,
optimisticResponse,
updater,
isMounted,
],
);

Expand Down
16 changes: 16 additions & 0 deletions tsconfig.esm.json
View file
Open in desktop
@@ -0,0 +1,16 @@
{
"compilerOptions": {
"outDir": "lib",
"rootDir": "src",
"module": "ESNext",
"target": "ES2020",
"moduleResolution": "node",
"noEmitOnError": true,
"declaration": true,
"lib": ["dom", "es6", "esnext.asynciterable", "es2017.object"],
"jsx": "react",
"skipLibCheck": true
},
"exclude": ["lib", "__tests__", "examples", "__mocks__", "coverage", "scripts"],
"compileOnSave": true
}
4 changes: 2 additions & 2 deletions tsconfig.json
View file
Open in desktop
@@ -1,12 +1,12 @@
{
"compilerOptions": {
"outDir": "lib",
"outDir": "lib/cjs",
"rootDir": "src",
"module": "commonjs",
"target": "es5",
"moduleResolution": "node",
"noEmitOnError": true,
"declaration": true,
"declaration": false,
"lib": ["dom", "es6", "esnext.asynciterable", "es2017.object"],
"jsx": "react",
"skipLibCheck": true
Expand Down