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

Fix types and exports of the package #197

Merged
merged 12 commits into from
Jan 4, 2024
Merged
5 changes: 5 additions & 0 deletions .changeset/strong-otters-brush.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"single-spa-react": patch
Copy link
Member

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.

Suggested change
"single-spa-react": patch
"single-spa-react": major

Copy link
Contributor Author

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:

image

Copy link
Member

@joeldenning joeldenning Oct 19, 2023

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to Major.

---

fix: types and exports of the package
MilanKovacic marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@ node_modules/
lib/
yarn-error.log
coverage
parcel/

# File is ignored as it is an identical copy of single-spa-react.d.ts, and is copied during the build
single-spa-react.d.cts
26 changes: 19 additions & 7 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,40 @@
"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.ts",
"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",
"build:types": "node scripts/build-types.js",
"lint": "eslint src",
"test": "concurrently pnpm:test:*",
"test:browser": "cross-env BABEL_ENV=test NODE_OPTIONS=--experimental-vm-modules jest --coverage",
Expand Down
20 changes: 20 additions & 0 deletions parcel/README.md
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).
4 changes: 4 additions & 0 deletions parcel/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"main": "../lib/cjs/parcel.cjs",
Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"types": "../types/parcel/index.d.cts"
}
10 changes: 10 additions & 0 deletions scripts/build-types.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import fs from "fs";

function copyFile(sourcePath, destinationPath) {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: calling fs.copyFileSync directly is simpler than extracting out a reusable copyFile function. Please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the script, as it is no longer needed.

fs.copyFileSync(sourcePath, destinationPath);
}

copyFile("types/single-spa-react.d.ts", "types/single-spa-react.d.cts");

// eslint-disable-next-line no-console
console.log("Build types completed.");
33 changes: 33 additions & 0 deletions types/parcel/index.d.cts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import * as React from "react";
joeldenning marked this conversation as resolved.
Show resolved Hide resolved
import {
ParcelConfig,
ParcelProps,
Parcel as SingleSpaParcel,
} from "single-spa";

export interface ParcelCompProps<ExtraProps = {}> {
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
> {}

// @ts-ignore
export = Parcel;