-
Notifications
You must be signed in to change notification settings - Fork 79
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
chore: update build system #1468
base: master
Are you sure you want to change the base?
Conversation
Size Change: -225 kB (-42.97%) 🎉 Total Size: 299 kB
|
7517db1
to
bedeada
Compare
bedeada
to
48f659b
Compare
48f659b
to
529fdf6
Compare
scripts/bundle-cjs.mjs
Outdated
const browserPlugin = { | ||
name: 'ignoreBrowser', | ||
setup: (build) => { | ||
const options = build.initialOptions; | ||
|
||
if (options.platform !== 'browser') return; | ||
|
||
build.onResolve({ filter: /(node:.+)|(jsonwebtoken)/ }, async ({ path }) => ({ path, external: 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.
I'm pretty sure this isn't the right approach, I'm going to have to find a similar way of doing what they did in Rollup:
stream-chat-js/rollup.config.js
Lines 15 to 19 in 7450517
const browserIgnore = { | |
name: 'browser-remapper', | |
resolveId: (importee) => (['jsonwebtoken', 'https', 'crypto'].includes(importee) ? importee : null), | |
load: (id) => (['jsonwebtoken', 'https', 'crypto'].includes(id) ? 'export default null;' : null), | |
}; |
(dropping imports for crypto
, jsonwebtoken
and https
altogether from browser 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.
Nevermind, this seems to work just as well. :)
4249c89
to
d05514f
Compare
3a561a8
to
c3a267d
Compare
c3a267d
to
0665ec1
Compare
"/src", | ||
"readme.md", | ||
"license" | ||
"/dist" |
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.
From npm documentation:
Certain files are always included, regardless of settings:
- package.json
- README
- LICENSE / LICENCE
- The file in the "main" field
- The file(s) in the "bin" field
Not sure why we were releasing sourcefiles as well though.
"@babel/plugin-proposal-class-properties": "^7.18.6", | ||
"@babel/plugin-proposal-object-rest-spread": "^7.20.7", | ||
"@babel/plugin-transform-object-assign": "^7.25.9", | ||
"@babel/plugin-transform-runtime": "^7.26.9", | ||
"@babel/preset-env": "^7.26.9", | ||
"@babel/preset-typescript": "^7.26.0", | ||
"@babel/register": "^7.25.9", |
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.
Unfortunately these are still needed for the Mocha to run properly, using different loader generated a TON of other issues I wasn't comfortable dealing with within this PR, updates should suffice for now.
"declarationDir": "./dist/types", | ||
"module": "commonjs", | ||
"target": "ES5" | ||
"module": "ES2020", |
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.
Note - important change here, the default output is now ESM.
"module": "commonjs", | ||
"target": "ES5" | ||
"module": "ES2020", | ||
"target": "ES6", |
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.
We should probably review the target too.
Description of the changes, What, Why and How?