-
Notifications
You must be signed in to change notification settings - Fork 21
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
DEV - Improve bundling and release #418
Conversation
* | ||
* This file is part of the tree-finder library, distributed under the terms of |
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 this was completely re-worked we do not need this heading
// To improve build times for large projects enable fork-ts-checker-webpack-plugin | ||
// const ForkTsCheckerWebpackPlugin = require("fork-ts-checker-webpack-plugin"); | ||
const isProd = process.env.NODE_ENV === "production"; | ||
const ASSET_PATH = isProd ? "" : "/"; |
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.
This is needed so that we can bundle the UI for conda-store-server
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.
This was much needed, thanks for doing this.
I'm going to take a closer look at this and test drive it locally, but in the meantime, I thought I could share some remarks and questions.
1. Create a new branch for the release `git checkout -b release-2024.9.1` | ||
1. Clean the branch `git clean -fxdq` | ||
1. Increment the version in `package.json` | ||
1. Build the package locally: |
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.
Why repeat the same info from the issue template?
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.
People sometimes look for the release.md file if curious vs. an issue template, so it would not hurt to keep this
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.
My concern is more about the repeated content, which then has to be updated in two places instead of one if these two documents are not going to get out of sync
enforce: "pre", | ||
exclude: /node_modules/, | ||
}, | ||
{ test: /\.js.map$/, use: "file-loader" }, |
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.
Seems like this one was deleted. Was it not needed?
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.
This has now been deprecated so moved to module assets
https://v4.webpack.js.org/loaders/file-loader/
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 not solid on any of this, but I thought the rules still had to stay in place, but the use: "file-loader"
could be replaced with type: "asset/resource"
or type: "asset"
. This is how you moved over the SVG rules.
I'm just pattern-matching here. I see that the SVG rules have been carried over, but not the bitmap rules, nor this .js.map
rule.
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.
Although I'm not sure about why .js.map
is needed because the source maps seemed to work in all of the different ways that I built the app after applying the code changes from this PR (not just webpack dev server mode).
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.
Aside from the existing docs suggestions, I'll defer to @gabalafou on webpack configuration changes. Thanks for this 🚀
Co-authored-by: gabalafou <gabriel@fouasnon.com>
Ahh, looks like playwright tests aren't passing - there's a Save button it seems to be waiting on. |
Hopefully PR #422 addresses the Playwright failure |
@trallard thanks for responding to all of my questions. I took this PR on three different test drives:
I clicked around. I logged in, logged out. I didn't kick off any new builds or create any new environments, but I don't see how this PR would affect that. So I doubt the Playwright failure is related, but if we want to be safe we could try merging in #422 first and then rebasing this PR. |
Fixes #408
Description
Changes in #389 broke the already brittle bundling of
conda-store
+conda-store-ui
.This pull request:
ui
side of the bundlingwebpack.config
(and the bundling overall)Pull request checklist
Additional information
Note
I did test this locally, the bundle seems fine and the app works as expected when running the Docker/out-of Docker stuff but I get this warning