-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fix types and exports of the package #197
Changes from 9 commits
2abc5fa
ac710c1
bb400e3
2a83c06
bb1aa45
898fd7b
9500fb6
6bf35de
de3a629
6a220c2
055387b
4b6fabe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"single-spa-react": patch | ||
--- | ||
|
||
fix: types and exports of the package | ||
MilanKovacic marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,5 +2,4 @@ node_modules/ | |
.DS_Store | ||
lib/ | ||
yarn-error.log | ||
coverage | ||
parcel/ | ||
coverage |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,34 +2,45 @@ | |
"name": "single-spa-react", | ||
"version": "5.1.4", | ||
"description": "Single-spa lifecycles helper for React apps", | ||
"main": "lib/umd/single-spa-react.js", | ||
"main": "lib/cjs/single-spa-react.cjs", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's change this back - we can discuss the reasons for changing to CJS in Slack and possibly in another PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted to UMD. |
||
"module": "lib/esm/single-spa-react.js", | ||
"type": "module", | ||
"exports": { | ||
"./package.json": "./package.json", | ||
".": { | ||
"import": "./lib/esm/single-spa-react.js", | ||
"require": "./lib/cjs/single-spa-react.cjs" | ||
"import": { | ||
"types": "./types/single-spa-react.d.ts", | ||
"default": "./lib/esm/single-spa-react.js" | ||
}, | ||
"require": { | ||
"types": "./types/single-spa-react.d.cts", | ||
"default": "./lib/cjs/single-spa-react.cjs" | ||
} | ||
}, | ||
"./parcel": { | ||
"import": "./lib/esm/parcel.js", | ||
"require": "./lib/cjs/parcel.cjs" | ||
"import": { | ||
"types": "./types/parcel/index.d.ts", | ||
"default": "./lib/esm/parcel.js" | ||
}, | ||
"require": { | ||
"types": "./types/parcel/index.d.cts", | ||
"default": "./lib/cjs/parcel.cjs" | ||
} | ||
} | ||
}, | ||
"types": "types/single-spa-react.d.cts", | ||
"files": [ | ||
"lib", | ||
"parcel", | ||
"types/single-spa-react.d.ts", | ||
"types", | ||
"README.md" | ||
], | ||
"tsd": { | ||
"directory": "src" | ||
}, | ||
"types": "types/single-spa-react.d.ts", | ||
"scripts": { | ||
"build": "concurrently pnpm:build:*", | ||
"build:rollup": "rollup -c", | ||
"build:types": "rimraf parcel && copyfiles -f types/parcel/index.d.ts ./parcel", | ||
"lint": "eslint src", | ||
"test": "concurrently pnpm:test:*", | ||
"test:browser": "cross-env BABEL_ENV=test NODE_OPTIONS=--experimental-vm-modules jest --coverage", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
# Parcel Folder | ||
|
||
## Overview | ||
This folder contains a `package.json` stub that redirects module resolutions, following the `package-json-redirects` strategy. | ||
|
||
## Why It's Here | ||
Serves as a compatibility layer for Node 10, which does not support the `exports` field in `package.json`. A separate `package.json` with `main` and `types` fields directs Node 10's resolution strategy to the appropriate files. | ||
|
||
```json | ||
{ | ||
"main": "./lib/cjs/parcel.cjs", | ||
"types": "./types/parcel/index.d.cts" | ||
} | ||
``` | ||
|
||
## How It Works | ||
When Node 10 attempts to import from this folder, it consults the `main` and `types` fields in `package.json` to locate the actual implementation and types files. This facilitates keeping those files in separate subfolders while making them accessible to older Node versions. | ||
|
||
## Reference | ||
For more detailed information and examples, see [this GitHub repository](https://github.com/andrewbranch/example-subpath-exports-ts-compat/tree/main/examples/node_modules/package-json-redirects). |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"main": "../lib/cjs/parcel.cjs", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The module format for this main field should match the format for the single-spa-react package.json (UMD) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
"types": "../types/parcel/index.d.cts" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,7 @@ const defaultOpts = { | |
unmountResolves: {}, | ||
}; | ||
|
||
export default function singleSpaReact(userOpts) { | ||
function singleSpaReact(userOpts) { | ||
if (typeof userOpts !== "object") { | ||
throw new Error(`single-spa-react requires a configuration object`); | ||
} | ||
|
@@ -375,3 +375,15 @@ function createSingleSpaRoot(opts) { | |
|
||
return SingleSpaRoot; | ||
} | ||
|
||
/** | ||
* If module is running in a CommonJS environment, a compatibility layer for node16 resolution strategy is inserted. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't add extra changes in subsequent commits |
||
* During transpilation, Rollup correctly inserts the "exports.default", and "exports.__esModule" properties, but they are not respected by some bundlers. | ||
* For more information see https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/CJSOnlyExportsDefault.md | ||
*/ | ||
if (typeof module !== "undefined" && module.exports) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove this, as discussed in the call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
module.exports = singleSpaReact; | ||
module.exports.default = singleSpaReact; | ||
} | ||
|
||
export default singleSpaReact; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import * as React from "react"; | ||
joeldenning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import { | ||
ParcelConfig, | ||
ParcelProps, | ||
Parcel as SingleSpaParcel, | ||
} from "single-spa"; | ||
|
||
interface ParcelCompProps<ExtraProps = {}> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be moved to a shared declarations file, and then imported into both the .d.ts and .d.cts files? import { ParcelCompProps } from './shared.d.ts'; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you agree, we can leave this as-is for now, since these files will not be needed when we migrate to typescript. |
||
config: ParcelConfig<ExtraProps>; | ||
mountParcel?: ( | ||
parcelConfig: ParcelConfig, | ||
parcelProps: ParcelProps & ExtraProps | ||
) => SingleSpaParcel; | ||
wrapWith?: string; | ||
wrapStyle?: React.CSSProperties; | ||
wrapClassName?: string; | ||
appendTo?: HTMLElement; | ||
parcelDidMount?: () => any; | ||
handleError?: (err: Error) => any; | ||
[extraProp: string]: any; | ||
} | ||
|
||
interface ParcelState { | ||
hasError: boolean; | ||
} | ||
|
||
declare class Parcel<ExtraProps = {}> extends React.Component< | ||
ParcelCompProps<ExtraProps>, | ||
ParcelState | ||
> {} | ||
|
||
export = Parcel; | ||
|
||
declare namespace Parcel { | ||
joeldenning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
export { ParcelCompProps, ParcelState }; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
import * as React from "react"; | ||
joeldenning marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import { AppProps, CustomProps, LifeCycleFn } from "single-spa"; | ||
|
||
declare const SingleSpaContext: React.Context<CustomProps & AppProps>; | ||
|
||
type DeprecatedRenderTypes = | ||
| "createBlockingRoot" | ||
| "unstable_createRoot" | ||
| "unstable_createBlockingRoot"; | ||
|
||
type LegacyRenderType = "hydrate" | "render"; | ||
|
||
type RenderType = "createRoot" | "hydrateRoot" | LegacyRenderType; | ||
|
||
interface SingleSpaReactOpts<RootComponentProps> { | ||
React: any; | ||
ReactDOM?: { | ||
[T in LegacyRenderType]?: any; | ||
}; | ||
ReactDOMClient?: { | ||
[T in RenderType]?: any; | ||
}; | ||
rootComponent?: | ||
| React.ComponentClass<RootComponentProps, any> | ||
| React.FunctionComponent<RootComponentProps>; | ||
loadRootComponent?: ( | ||
props?: RootComponentProps | ||
) => Promise<React.ElementType<typeof props>>; | ||
errorBoundary?: ( | ||
err: Error, | ||
errInfo: React.ErrorInfo, | ||
props: RootComponentProps | ||
) => React.ReactElement; | ||
errorBoundaryClass?: React.ComponentClass<RootComponentProps>; | ||
parcelCanUpdate?: boolean; | ||
suppressComponentDidCatchWarning?: boolean; | ||
domElementGetter?: (props: RootComponentProps) => HTMLElement; | ||
renderType?: RenderType | (() => RenderType); | ||
} | ||
|
||
interface ReactAppOrParcel<ExtraProps> { | ||
bootstrap: LifeCycleFn<ExtraProps>; | ||
mount: LifeCycleFn<ExtraProps>; | ||
unmount: LifeCycleFn<ExtraProps>; | ||
update?: LifeCycleFn<ExtraProps>; | ||
} | ||
|
||
declare function singleSpaReact<ExtraProps = {}>( | ||
opts: SingleSpaReactOpts<ExtraProps & AppProps> | ||
): ReactAppOrParcel<ExtraProps>; | ||
|
||
export = singleSpaReact; | ||
|
||
declare namespace singleSpaReact { | ||
export { | ||
SingleSpaContext, | ||
DeprecatedRenderTypes, | ||
LegacyRenderType, | ||
RenderType, | ||
SingleSpaReactOpts, | ||
ReactAppOrParcel, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever the
exports
field changes in the package.json, it might need to be a major change because of how different bundlers and NodeJS versions handle it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix should handle all resolution strategies (all typescript strategies), along with all popular bundlers. Still, if someone is using an obscure tool, it might behave differently, so we could treat it as a breaking change. Problem is not only with the typescript version, but a combination of typescript "moduleResolution", and bundlers. Currently, create-single-spa uses the "node" (which is actually node10) resolution strategy. What do you think?
I have inserted a compatibility layer that fixes the issue with certain bundlers requiring access to default export with ".default". Now, all of the resolution strategies are correctly handled:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being totally honest, I would prefer calling it a major version just in case there's a weird edge case where it changes how things work. I don't know all the edge cases, but have had lots of github issues after changing my
exports
fields before.I prefer single-spa-react users face no risk when doing patch upgrades.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to Major.