-
Notifications
You must be signed in to change notification settings - Fork 8
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: use two-step build #115
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
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.
self review
.aegir.js
Outdated
build: { | ||
config: { | ||
banner: {}, | ||
footer: {}, | ||
entryPoints: [`${paths.src}/src/index.jsx`, `${paths.src}/src/sw.js`], | ||
minify: false, | ||
splitting: true, | ||
format: 'esm', | ||
outdir: paths.dist, | ||
chunkNames: 'ipfs-sw-[name]', | ||
loader: { | ||
'.svg': 'dataurl' | ||
}, | ||
sourcemap: true, | ||
plugins: [copyPlugin], | ||
define: { 'process.env.NODE_ENV': '"production"' } | ||
} | ||
}, |
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 don't need the results of this.
"test:browsers": "playwright test -c playwright.config.js", | ||
"test:chrome": "playwright test -c playwright.config.js --project chromium", | ||
"test:firefox": "playwright test -c playwright.config.js --project firefox", | ||
"test:node": "aegir test -t node -f dist-tsc/test/node.js --cov", |
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.
@achingbrain I could probably use your aegir expertise on why aegir isn't recognizing the node.js file is only for nodejs tests
patches/aegir+42.2.2.patch
Outdated
@@ -0,0 +1,42 @@ | |||
diff --git a/node_modules/aegir/src/build/index.js b/node_modules/aegir/src/build/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.
created issue in ipfs/aegir: ipfs/aegir#1481.
will remove.
extensionAlias: { | ||
'.js': ['.js', '.ts'], | ||
'.jsx': ['.jsx', '.tsx'], | ||
'.cjs': ['.cjs', '.cts'], | ||
'.mjs': ['.mjs', '.mts'] | ||
} |
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.
fixes error in webpack when using .js imports inside TS files during npm start
pinging folks so they know... @2color @lidel @aschmahmann if you've got code changes in this repo, you may be in for an annoying rebase.. but things seem to be looking good so i'm merging before folks get a chance to work on something that will be swept out from under them. |
Creates a two step build process so we can benefit from test environment that aegir gives us without having to finagle a ton of other test env setup.
Ideally, we would use only aegir for building, but aegir doesn't do us any favor building web assets
Tasks