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

[TASK] Migrate to Vite #46

Merged
merged 6 commits into from
Oct 10, 2023
Merged

[TASK] Migrate to Vite #46

merged 6 commits into from
Oct 10, 2023

Conversation

weeman1337
Copy link
Collaborator

@weeman1337 weeman1337 commented Sep 22, 2023

  • Migrates to
    • Vite
    • ES modules
  • Adds prettier 💄
  • Migrates to ESLint recommended

closes #13

Description + Motivation and Context

This PR migrates the build system from Gulp to Vite. Gulp is rarely maintained, the packages become outdated and it becomes more and more problematic to keep the dependencies up to date. Good example are the overrides to „fix“ the dependencies security issues.

Because this PR touches every file anyway 🤷 It also introduces Prettier for a consistent code-style.

How Has This Been Tested?

Tested builds locally.

Screenshots/links:

No user-facing change

Checklist:

  • My code follows the code style of this project. (CI will test it anyway and also needs approval)
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.

@weeman1337 weeman1337 added enhancement New feature or request dependencies Pull requests that update a dependency file github_actions Pull requests that update GitHub Actions code javascript Pull requests that update Javascript code labels Sep 22, 2023
@MyIgel

This comment was marked as resolved.

Signed-off-by: weeman <[email protected]>
Add ESLint to scripts. Make polyfill ESLint compliant.

Signed-off-by: weeman <[email protected]>
@weeman1337 weeman1337 force-pushed the weeman1337/build branch 3 times, most recently from ae7f076 to 820793f Compare October 2, 2023 10:19
@weeman1337 weeman1337 marked this pull request as ready for review October 2, 2023 10:35
Copy link
Member

@MyIgel MyIgel left a comment

Choose a reason for hiding this comment

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

That setup works so great! 😍

Some preliminary findings:

  • npm install shows warnings, can they easily be fixed?

    npm WARN deprecated @stylelint/[email protected]: Use the original unforked package instead: postcss-markdown
    npm WARN deprecated [email protected]: Please use @jridgewell/sourcemap-codec instead
    npm WARN deprecated [email protected]: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-terser
    npm WARN deprecated @stylelint/[email protected]: Package no longer supported. Contact Support at https://www.npmjs.com/support for more info.
    npm WARN deprecated [email protected]: babel-eslint is now @babel/eslint-parser. This package will no longer receive updates.
    
  • There seem to be some possible moderate vulnerabilities, are they easily fixable?

  • When opening a node the left side panel is blank and the console shows an error.

  • When opening the node graph, the page seems to work but shows an error in the console

either way it might have been easier to recview if the linting vs migration would have been two MRs ;)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@MyIgel MyIgel left a comment

Choose a reason for hiding this comment

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

Those "fixes" might fix the above errors but might be implemented somehow better, dunno :D

Also I'm wondering if there is an easy option to keep the whole graph centred all the time, even before it gets rescaled to zoom to a specific node?

lib/forcegraph.js Show resolved Hide resolved
lib/infobox/node.js Outdated Show resolved Hide resolved
Signed-off-by: weeman <[email protected]>
@weeman1337
Copy link
Collaborator Author

Errors are fixed. Removed and updated some packages.

Two deprecation warnings are left: #49

Also I'm wondering if there is an easy option to keep the whole graph centred all the time, even before it gets rescaled to zoom to a specific node?

Can you fill in details here #50 ?

Copy link
Member

@MyIgel MyIgel left a comment

Choose a reason for hiding this comment

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

That looks better, just a small hickup:

README.md Show resolved Hide resolved
@MyIgel

This comment was marked as resolved.

public/config.json Outdated Show resolved Hide resolved
vite.config.js Show resolved Hide resolved
MyIgel

This comment was marked as outdated.

Copy link
Member

@skorpy2009 skorpy2009 left a comment

Choose a reason for hiding this comment

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

LGTM
works for me

@skorpy2009 skorpy2009 merged commit 2ee5ee8 into main Oct 10, 2023
1 check passed
@skorpy2009 skorpy2009 deleted the weeman1337/build branch October 10, 2023 14:04
@skorpy2009
Copy link
Member

Danke @weeman1337

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request github_actions Pull requests that update GitHub Actions code javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate away from Gulp
3 participants