Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: defer loading of pretty-error to improve startup time #1789

Merged
merged 3 commits into from
Apr 15, 2023

Conversation

Knagis
Copy link
Contributor

@Knagis Knagis commented Apr 14, 2023

Fixes #1788

@alexander-akait
Copy link
Collaborator

@Knagis Can you fix linting?

@Knagis
Copy link
Contributor Author

Knagis commented Apr 14, 2023

fixed

Copy link
Collaborator

@TheLarkInn TheLarkInn left a comment

Choose a reason for hiding this comment

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

Nice improvement! Thanks for the contribution 🙇

@Knagis
Copy link
Contributor Author

Knagis commented Apr 14, 2023

the failures are caused by timeouts in tests. locally the tests passed green in around a minute..

@alexander-akait
Copy link
Collaborator

@Knagis Looks like we need update some snapshots, can you do it ehre?

@alexander-akait
Copy link
Collaborator

@Knagis
Copy link
Contributor Author

Knagis commented Apr 14, 2023

hmm.. you're right

    - /* harmony default export */ const __WEBPACK_DEFAULT_EXPORT__ = (_node_modules_css_loader_dist_cjs_js_main_css__WEBPACK_IMPORTED_MODULE_1__/* ["default"].locals */ .Z.locals || {});
    + /* harmony default export */ const __WEBPACK_DEFAULT_EXPORT__ = (_node_modules_css_loader_dist_cjs_js_main_css__WEBPACK_IMPORTED_MODULE_1__/* .default.locals */ .Z.locals || {});

but locally on node 16 all snapshots match

@alexander-akait
Copy link
Collaborator

@Knagis Yeah, it is very strange, do I need to look why it happens (maybe updating them to solve the problem on CI, because we don't have node.js v16 here and it is not critical change)?

@Knagis
Copy link
Contributor Author

Knagis commented Apr 14, 2023

switched to node12, still everything passes..

 PASS  spec/example.spec.js
  HtmlWebpackPlugin Examples
    √ custom-template example (667 ms)
    √ default example (142 ms)
    √ favicon example (116 ms)
    √ html-loader example (157 ms)
    √ inline example (210 ms)
    √ pug-loader example (153 ms)
    √ javascript example (115 ms)
    √ javascript-advanced example (118 ms)
    √ sort manually example (129 ms)
    √ multi-page example (107 ms)
    √ template-parameters example (67 ms)

@Knagis
Copy link
Contributor Author

Knagis commented Apr 14, 2023

hmm. now running npm install --ignore-scripts --force --legacy-peer-deps webpack@5 that the CI runs (previously installed deps via yarn since npm i complained about something). Will test with that

@Knagis
Copy link
Contributor Author

Knagis commented Apr 14, 2023

So running that modifies dependency:

-    "webpack": "5.24.3",
+    "webpack": "^5.79.0",

Why would CI force newest webpack instead of using the one in package.json?

Should i perhaps commit this updated package.json together with fresh snapshots?

@Knagis
Copy link
Contributor Author

Knagis commented Apr 14, 2023

ok, so ended up making more changes:

  • fixed example.spec.js to correctly return errors and not get stuck with timeout
  • updated webpack in package.json to newest - to match the version currently used by CI
  • updated typescript because webpack types failed with the previous version (using 4.9, not 5.0 because ts5 bumped node requirements)
  • updated example snapshots

@alexander-akait
Copy link
Collaborator

Thank you

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

Successfully merging this pull request may close these issues.

lazy-require pretty-error to improve startup time
3 participants