-
Notifications
You must be signed in to change notification settings - Fork 41
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(bundling): upgrades deps and bundling #130
Conversation
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.
Thank you so much for the pr! I left some comments below
7bd3c7c
to
e46c064
Compare
"settings": { | ||
"svelte3/typescript": true | ||
"import/no-unresolved": 0, | ||
"no-inner-declarations": 0 |
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 guessing this one was required so there weren't a ton of errors introduced?
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 was only one error, which was the one I rolled back. I figured we could always fix it later and remove the lint rule.
@dysfunc Can you please fix the lock file as the build is failing. Might be good to update the actions as well as I think the latest story book requires node 18+. Would just be good to target LTS (v20) |
@dysfunc I think we might need to upgrade npm actions to node 20, I can't even run master branch locally due to esm failures. |
What errors are you seeing? I can install, build, run storybook on 16+ |
There were precommit hooks were failing for me with a lot of esm related messages. Looks like two of the github actions are failing, one of them with esm message. Thanks again for all of your help. I think the deps being so out of date is making this harder than normal. That will be the next thing I handle after this pr |
I fixed the size test. The only one left is the editorconfig. It's failing on |
Thanks, I'm going to merge this and then fix that in a separate pr. |
@dysfunc just noticed this https://github.com/SauravKanchan/svelte-chartjs/actions/runs/7716442819/job/21033255322 All good! I can't think of anything off the top of my head that would cause this other than possibly the old process was outputting a package file? |
the old process did copy package.json. the new process doesn't; it uses the root package. |
This is the first time I've used pnpm, kinda looks like we need to copy it. |
it shouldn't be copied over. I'm looking at it right now |
i think the
|
It used to be there for the package folder?
|
I would remove that and rerun publish |
This includes:
storybook
to7.6.10
vite
to5.0.12
vitest
to1.2.1
eslint
, and related pluginssvelte
to4.2.9
prettier
to3.0.1
@sveltejs/package
and includespublint
for linting bundles.dist
exports
in package.jsonI've tested the new bundling with my company's production web app, and it works as expected. Please do your own testing as well :)
This was the initial PR #128 @niemyjski