Skip to content
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

fix: Update flatpak-builder dependencies, fixes #214 #215

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jochumdev
Copy link

This fixes issue #214 by updating dependencies of flatpak-builder.

Thanks for the great action!

Kind regards,
René

@Totto16
Copy link
Contributor

Totto16 commented Feb 5, 2025

Just a question, this repo never used webpack, since there is only one index.js, there is no need for a bundler, why did you add a webpack configuration? I think that never makes it past an eventual review 😓

Copy link
Contributor

@Totto16 Totto16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all webpack related stuff 😓

(Side note, I am not a member of this org, but since nobody reviewed this after 5 days, I just reviewed this PR, actual maintainers might have different opinions than me, but I'll say that my suggestions are at least sound by the practices of the rest of the repo)

@@ -391,11 +391,11 @@ const run = async (config) => {
return
}

const artifactClient = artifact.create()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name artifactClient fits better, just use that name (despite the docs at https://www.npmjs.com/package/@actions/artifact using artifact )

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be done.

"js-yaml": "^4.1.0"
},
"devDependencies": {
"eslint": "^8.49.0",
"eslint": "^9.19.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONT update eslint, it changed the entire config format between major versions. If you run npx eslint . after this you'll get this error:

Oops! Something went wrong! :(

ESLint: 9.19.0

ESLint couldn't find an eslint.config.(js|mjs|cjs) file.

From ESLint v9.0.0, the default configuration file is now eslint.config.js.
If you are using a .eslintrc.* file, please follow the migration guide
to update your configuration file to the new format:

https://eslint.org/docs/latest/use/configure/migration-guide

If you still have problems after following the migration guide, please stop by
https://eslint.org/chat/help to chat with the team.
``

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That as wel.

@jochumdev
Copy link
Author

Just a question, this repo never used webpack, since there is only one index.js, there is no need for a bundler, why did you add a webpack configuration? I think that never makes it past an eventual review 😓

I wasn't sure howto build that dist/index.js - so for the quick one i used webpack as everywhere in that file webpack was referenced (because of loaders ofc.).

Will remove that.

@jochumdev jochumdev force-pushed the builder-update-deps branch from 688fe90 to f93378a Compare February 5, 2025 17:35
@jochumdev
Copy link
Author

@Totto16 can you please build dist/index.js somehow and upload it to that branch?

@Totto16
Copy link
Contributor

Totto16 commented Feb 5, 2025

@Totto16 can you please build dist/index.js somehow and upload it to that branch?

After some deep dive I think you weren't that wrong with the webpack file, the generated dist/index.js file really looks like it was generated by webpack 😓 My bad for not catching that, as said I am no real maintainer of this repo 😓

I think you just can use the webpack config you used, install webpack and webpack-cli globally with e.g. npm i -g webpack webpack-cli and then just commit the changes of dist/index.js

@jochumdev
Copy link
Author

I think you just can use the webpack config you used, install webpack and webpack-cli globally with e.g. npm i -g webpack webpack-cli and then just commit the changes of dist/index.js

What do you think about adding webpack back to the repo, so it's documented.

@Totto16
Copy link
Contributor

Totto16 commented Feb 5, 2025

I think you just can use the webpack config you used, install webpack and webpack-cli globally with e.g. npm i -g webpack webpack-cli and then just commit the changes of dist/index.js

What do you think about adding webpack back to the repo, so it's documented.

For that a real maintainer needs to explain, why it isn't present in the first place... I find that a good idea, but maybe do that in a separate PR, so that this just has the needed fixes and can be merged faster, and the discussion about this might take a while

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants