-
-
Notifications
You must be signed in to change notification settings - Fork 930
Upgrade dependencies, add package entrypoints #824
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
Conversation
File size impactError: Error while trying to generate a snapshot for 6.0-deps merge into 6.0. Error: Command failed: git merge FETCH_HEAD --allow-unrelated-histories at ChildProcess.exithandler (child_process.js:303:12) at ChildProcess.emit (events.js:315:20) at maybeClose (internal/child_process.js:1026:16) at Process.ChildProcess._handle.onexit (internal/child_process.js:286:5) Generated by file size impact during file-size-impact#1005399717 |
"types": "typings/single-spa.d.ts", | ||
"type": "module", | ||
"exports": { |
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.
I tested these in Node 10, Node 16 (esm importing esm, cjs importing esm, and cjs importing cjs, both for prod and dev via node --conditions=development
). I also tested it in webpack 4, webpack 5, and rollup (with @rollup/plugin-node-resolve
). Seems to work.
@@ -11,6 +11,7 @@ const useAnalyzer = process.env.ANALYZER === "analyzer"; | |||
const replaceOpts = { | |||
"process.env.BABEL_ENV": null, | |||
__DEV__: !isProduction, | |||
preventAssignment: true, |
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.
Rollup recommended setting this to prepare for an upcoming breaking chagne.
rollup.config.js
Outdated
@@ -33,7 +34,7 @@ export default (async () => [ | |||
input: "./src/single-spa.js", | |||
output: [ | |||
{ | |||
file: `./lib/umd/single-spa${isProduction ? ".min" : ".dev"}.js`, | |||
file: `./lib/umd/single-spa${isProduction ? ".min" : ".dev"}.cjs`, |
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.
Since the package.json "type"
is now "module"
, the umd build needs the CJS extension
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.
I guess the alternative that I've seen packages do, is to put another package.json
with the contents of {type: 'commonjs'}
in the umd
folder so that you can maintain the .js
extension.
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.
I don't mind the cjs extension. It's not super easy to do echo '{\"type\": \"commonjs\"}' > lib/umd/package.json
in a fully platform agnostic way - the echo part sometimes creates invalid json characters in windows, and also the file separators are sometimes tricky. I could do it with node, but it's sort of a pain to have to create a build script just to do this.
done(); | ||
} | ||
}) | ||
.catch(fail); |
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.
fail
no longer exists in jest 26/27
@@ -2,7 +2,7 @@ import * as singleSpa from "single-spa"; | |||
|
|||
describe("partial rerouting", () => { | |||
function delay() { | |||
return new Promise((resolve) => setImmediate(resolve)); |
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.
setImmediate has issues in jest 27. See jestjs/jest#11539
@@ -24,6 +24,10 @@ describe("parcel errors", () => { | |||
const parcel1 = app.mountProps.mountParcel(parcelConfig1, { | |||
domElement: document.createElement("div"), | |||
}); | |||
|
|||
// avoid unhandled rejection causing test failure |
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.
Required for NodeJS 16 promise stuff
package.json
Outdated
"exports": { | ||
".": { | ||
"development": { | ||
"import": "./lib/esm/single-spa.dev.js", |
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.
I'm using the esm
directory rather than es2015
to maintain IE11 support. One option could be to change it to es2015
here and then ask users who support IE11 to use the esm
version, as a step towards dropping IE11 support. Thoughts?
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.
What's the size difference between the two?
In any case, I think it may be worth dropping IE11 support by default - otherwise, we're stuck waiting for the next major to drop it, and who knows how long that will be?
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.
It's 2kb. I agree it would be nice to start indicating that we're not going to support IE11 forever by changing the default to not support IE11 but still have it supported with a bit of manual configuration.
Most people use the system
version, though, which currently supports ie11. Maybe we could create a system2015 version, as well?
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.
Am leaning towards this directory structure:
lib/
es5/
umd/
system/
esm/
es2015/
umd/
system/
esm/
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.
That sounds good to me
package.json
Outdated
"exports": { | ||
".": { | ||
"development": { | ||
"import": "./lib/esm/single-spa.dev.js", |
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.
What's the size difference between the two?
In any case, I think it may be worth dropping IE11 support by default - otherwise, we're stuck waiting for the next major to drop it, and who knows how long that will be?
@@ -2,25 +2,46 @@ | |||
"name": "single-spa", | |||
"version": "5.9.3", |
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.
If this is the 6 branch, may be worth updating this right now too?
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.
I only update version during publish, since pnpm version major
automatically updates package.json
rollup.config.js
Outdated
@@ -33,7 +34,7 @@ export default (async () => [ | |||
input: "./src/single-spa.js", | |||
output: [ | |||
{ | |||
file: `./lib/umd/single-spa${isProduction ? ".min" : ".dev"}.js`, | |||
file: `./lib/umd/single-spa${isProduction ? ".min" : ".dev"}.cjs`, |
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.
I guess the alternative that I've seen packages do, is to put another package.json
with the contents of {type: 'commonjs'}
in the umd
folder so that you can maintain the .js
extension.
} | ||
}) | ||
.catch(fail); | ||
it(`is fired on the window whenever the hash changes`, async () => { |
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.
I'm pretty sure you can still do async (done) =>
if that makes things easier. That way you don't have to do the finish
promise trick
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.
No, you can't, according to https://jestjs.io/blog/2021/05/25/jest-27#features-coming-with-breaking-changes
Merging this now for a beta release of 6.0. Am happy to address any more feedback in future PRs |
This includes breaking changes and will be part of single-spa@6. It's to the 6.0 branch
This resolves #652 and makes it possible to easily resolve single-spa/single-spa-layout#141 (by publishing a new major version of single-spa-layout that uses the package entrypoints).
Included is the following:
pinst
to avoid issues with yarn 2 users as described in https://typicode.github.io/husky/#/?id=yarn-2"type"
to"module"
"exports"
.cjs
extension for theumd
folder. This is because when"type"
is"module"
the.js
files must be ESM, but the UMD build is not. Using cjs works around this."main"
to have the.cjs
extensionbabel-eslint
dependency@rollup/plugin-node-resolve
instead ofrollup-plugin-node-resolve
)