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

Resolve remaining build warnings #113

Closed
5 tasks done
shellyear opened this issue Mar 6, 2024 · 11 comments
Closed
5 tasks done

Resolve remaining build warnings #113

shellyear opened this issue Mar 6, 2024 · 11 comments
Assignees

Comments

@shellyear
Copy link
Collaborator

shellyear commented Mar 6, 2024

Fix remaining warnings left in here

Note: remaining warnings left after npm run validate are actually build-related. I've also included fixes of runtime errors shown in the browser, I can cherry-pick to a separate branch if needed.

solving in this PR

Warnings:

Screenshot 2024-03-06 at 19 20 34
  • 2) UPD: seems to be related to rollup, not the eslint
Screenshot 2024-03-06 at 19 22 32 Screenshot 2024-03-06 at 19 23 17
  • 4)
Screenshot 2024-03-06 at 19 46 46
  • 5)
Screenshot 2024-03-08 at 11 18 12
@shellyear
Copy link
Collaborator Author

It seems that the 3rd error is related to datatables.net package

@blcham blcham changed the title Remaining warnings in feature/eslint Resolve remaining warnings created by configuring eslint Mar 6, 2024
@blcham
Copy link

blcham commented Mar 6, 2024

@LaChope note that some of the issues created in #109 were not resolved yet and when running eslint it might give you warning as described here.

@shellyear
Copy link
Collaborator Author

shellyear commented Mar 7, 2024

Hi! @LaChope The [2] warning is related to the @kbss-cvut/s-forms package, in the dist folder of this module in s-forms.modern.js this error appears "node_modules/@kbss-cvut/s-forms/dist/s-forms.modern.js" contains an annotation that Rollup cannot interpret due to the position of the comment. The comment will be removed to avoid issues.".

Options:

  1. We can solve it by defining the rollup.config.js and ignore these errors
  2. Or try to solve the root cause in the @kbss-cvut/s-forms package

@LaChope
Copy link
Collaborator

LaChope commented Mar 7, 2024

From this discussion, I think it can be simply ignored (configured so that we don't see this message anymore).

@shellyear
Copy link
Collaborator Author

shellyear commented Mar 7, 2024

From this discussion, I think it can be simply ignored (configured so that we don't see this message anymore).

ok, thanks, I'll try to filter them out via rollup config

UPD: it is possible to configure in vite.config.js

@shellyear shellyear changed the title Resolve remaining warnings created by configuring eslint Resolve remaining build warnings Mar 8, 2024
@shellyear
Copy link
Collaborator Author

shellyear commented Mar 8, 2024

Hi, @blcham, @LaChope. All the errors were either fixed or filtered out from showing, also the [5.1] warning is related to store npm package being used in the dependencies in @kbss-cvut/s-forms@0.7.1-beta-48c6cc4.0
Screenshot 2024-03-08 at 11 18 48

I'll filter this out in vite.config.js and leave a comment in the code referencing this issue

@shellyear
Copy link
Collaborator Author

shellyear commented Mar 14, 2024

@blcham
[1] fixed - was solved by using pofyliss in aliases in vite.config.js
[2] in progress - the root cause need to be solved % - see reply below
[3] in progress - the root cause need to be solved % - see reply below
[4] fixed- common runtime error, fixed proptypes
[5.1] fixed - "can't be bundle without module attribute" - fixed by using slash in src="/config.js" - works with docker, build and dev
[5.2] - in progress - relates to "store" package used in @kbss-cvut/s-forms - see reply below

@shellyear
Copy link
Collaborator Author

shellyear commented Mar 14, 2024

@blcham The [2] error happens during the tree shaking made by rollup in vite.

  • In "node_modules/@kbss-cvut/s-forms/dist/s-forms.modern.js" contains an annotation that Rollup cannot interpret due to the position of the comment. The comment will be removed to avoid issues.
  • #PURE*/ annotation is a hint to the minifier that the associated code can be safely removed if it's not being used.
  • Invalid annotations are removed and Rollup emits a warning. Valid annotations remain in the code unless their function call or constructor invocation is removed as well.

Since the wrongly placed annotation is node_modules/@kbss-cvut/s-forms/dist/s-forms.modern.js, then I will try to solve it directly in that package's repo.

%

@shellyear
Copy link
Collaborator Author

shellyear commented Mar 14, 2024

Hi, @blcham, @LaChope. All the errors were either fixed or filtered out from showing, also the [5.1] warning is related to store npm package being used in the dependencies in @kbss-cvut/s-forms@0.7.1-beta-48c6cc4.0 Screenshot 2024-03-08 at 11 18 48

I'll filter this out in vite.config.js and leave a comment in the code referencing this issue

@blcham [5.2] Use of eval in "node_modules/store/plugins/lib/json2.js" is strongly discouraged as it poses security risks and may cause issues with minification

The root cause is the "eval" function being used inside node_modules/store/plugins/lib/json2.js. The "store" package is being used inside of @kbss-cvut/s-forms, in one of it's dependencies. So the solution will be to not rely on packages that use eval, or just simply filter out this error. Also eval by itself is not recommended to use in js, due to security issues

@shellyear
Copy link
Collaborator Author

shellyear commented Mar 14, 2024

@blcham > It seems that the [3] error is related to datatables.net package

Screenshot 2024-03-14 at 18 37 44 %
Screenshot 2024-03-14 at 18 41 57

  • The package comes with its prebuild minified css, which is then add to the final minified css file. The purpose of asterisk (*) is hack for IE . So basically it is not an actual error, but workaround by datatables.net, but since Vite aims to support moderns browsers, and does not support IE by default, this warning is being thrown.
  • *property: value_ applies the property value in IE 7 and below. It may or may not work in future versions. Warning: this uses invalid CSS.
  • datatables.net also relies on JQuery, and using packages for jquery with react is discouraged

To avoid this warning, we should avoid direct or indirect use of this package. Or I can try to modify that minified css coming from that package separately via postcss, and emit it either as a part of single minified file or as a separate minified css file

@blcham
Copy link

blcham commented Mar 15, 2024

Good job.

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

No branches or pull requests

3 participants