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
Version 8: Build changes - ES2022, no more tslib, etc. etc. #7261
Conversation
We were not running the tests anyhow.
+ Removing umd.ts file
LMAO wut?!
+ Removes our only dependency! We don't need it anymore because we are targeting modern JS. + Removes `check-side-effects`, which is yet another Angular side-project they stopped supporting. This is because `check-side-effects` doesn't support modern JS features. I'm not going to replace this with anything for the time being.
@@ -19,50 +18,43 @@ | |||
"types": "./dist/types/index.d.ts", | |||
"node": "./dist/cjs/index.js", | |||
"require": "./dist/cjs/index.js", | |||
"es2015": "./dist/esm/index.js", | |||
"default": "./dist/esm5/index.js" | |||
"default": "./dist/esm/index.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.
There is no "type": "module"
in package.json
, so the file specified in "default"
will be parsed as CommonJS, right? But there is a file with ES modules.
".": {
"types": "./dist/types/index.d.ts",
"node": "./dist/cjs/index.js",
"import": "./dist/esm/index.js",
"default": "./dist/cjs/index.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.
Also, maybe you should move "node"
down or remove it altogether?
"node"
- matches for any Node.js environment. Can be a CommonJS or ES module file. In most cases explicitly calling out the Node.js platform is not necessary.
import {timer} from 'rxjs'; // version from current PR
timer(2000).subscribe(() => {
console.trace('2000');
});
% node main.js
Trace: 2000
at Object.next (file:///example/main.js:4:11)
at ConsumerObserver.next (example/node_modules/rxjs/dist/cjs/internal/Subscriber.js:99:33)
at Subscriber._next (example/node_modules/rxjs/dist/cjs/internal/Subscriber.js:70:26)
at Subscriber.next (example/node_modules/rxjs/dist/cjs/internal/Subscriber.js:41:18)
at AsyncAction.work (example/node_modules/rxjs/dist/cjs/internal/observable/timer.js:28:28)
at AsyncAction._execute (example/node_modules/rxjs/dist/cjs/internal/scheduler/AsyncAction.js:79:18)
at AsyncAction.execute (example/node_modules/rxjs/dist/cjs/internal/scheduler/AsyncAction.js:67:26)
at AsyncScheduler.flush (example/node_modules/rxjs/dist/cjs/internal/scheduler/AsyncScheduler.js:38:33)
at listOnTimeout (node:internal/timers:573:17)
at process.processTimers (node:internal/timers:514:7)
NodeJS imports the CommonJS (cjs) version instead of the ES version.
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.
Honestly, I didn't change anything that was already working. It's still working, this is the minimal change to get the right JS target, I don't want to change it unless it's breaking someone.
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.
Agree. I will try to do a thorough research and make a separate issue for this.
Co-authored-by: Dmitry Demensky <10235949+demensky@users.noreply.github.com>
Please review by commit, there are several changes and cleanups made.
Highlights:
tslib
!!es2015
foldercheck-side-effects
!NOTE:
check-side-effects
is yet another unmaintained side project (I was badgered into using) we depended on from the Angular team. It broke when we updated to output ES2022, because it couldn't handled things like?.
. There are no more updates for us to pull from them, we were on the latest version.