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

Conversation

MilanKovacic
Copy link
Contributor

@MilanKovacic MilanKovacic commented Oct 14, 2023

Fixes #170.
This fix employs several strategies to achieve compatibility with all resolution strategies (node10, node16 - cjs, node16 - esm, bundler).

  • Exports are split into import and require sections — required due package having a type "module" in package.json
  • Types of the main export (single-spa-react.d.ts) are copied during the build process — required by TypeScript (one file can not represent types of both CJS and ESM export, even if they are identical)
  • Additional types (index.d.cts) for parcel CJS format are added. This file is not identical to the index.d.ts (see the last export line).
  • For node10 resolution strategy, parcel folder containing package.json redirects is inserted into the build. This is required for tooling that does not support subexports. Such tools will look into the "parcel" folder when doing "import ... from 'single-spa-react/parcel", locate the package.json, read the "main"/"types" field and be able to locate the CJS export (such tooling only supports CJS format).

Here is the analysis by https://arethetypeswrong.github.io/.
NOTE:
Due to rollup not inserting a compatibility layer when transpiling, in rare instances, consumers might have to access the main export with ".default". This is caused by a mix of named, and default exports.

image

@changeset-bot
Copy link

changeset-bot bot commented Oct 14, 2023

🦋 Changeset detected

Latest commit: 4b6fabe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
single-spa-react Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@robmosca robmosca left a comment

Choose a reason for hiding this comment

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

Tested on our code base and it seems to work well. 👍🏽

Copy link
Member

@joeldenning joeldenning left a comment

Choose a reason for hiding this comment

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

This is a risky change. Changing the exports field is almost always a major breaking change, due to how different versions of Node, bundlers, etc handle the exports field. What version of typescript started supporting the exports field rather than types in package.json?

NOTE:
Due to rollup not inserting a compatibility layer when transpiling, in rare instances, consumers might have to access the main export with ".default". This is caused by a mix of named, and default exports.

The above sounds like it might be a breaking 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.

@@ -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.

types/parcel/index.d.cts Show resolved Hide resolved
package.json Outdated
@@ -2,7 +2,7 @@
"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",
Copy link
Member

Choose a reason for hiding this comment

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

Changing the main field from UMD to CJS is a breaking change. Please update the changeset to be a major

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to UMD.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Please don't add extra changes in subsequent commits

types/parcel/index.d.cts Show resolved Hide resolved
types/single-spa-react.d.cts Show resolved Hide resolved
Parcel as SingleSpaParcel,
} from "single-spa";

interface ParcelCompProps<ExtraProps = {}> {
Copy link
Member

@joeldenning joeldenning Nov 7, 2023

Choose a reason for hiding this comment

The 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';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -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.

* 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) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this, as discussed in the call

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.

@joeldenning joeldenning merged commit 1141959 into main Jan 4, 2024
2 checks passed
@joeldenning joeldenning deleted the fix/types-and-exports branch January 4, 2024 22:59
@github-actions github-actions bot mentioned this pull request Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

package.json exports when using moduleResolution node16
4 participants