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: 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

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.

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