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: support serialization of RegExp #102

Merged
merged 3 commits into from
Apr 10, 2021
Merged

fix: support serialization of RegExp #102

merged 3 commits into from
Apr 10, 2021

Conversation

qnighy
Copy link
Contributor

@qnighy qnighy commented Sep 30, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Part of #75.

enhanced-resolve v4.2.0 introduced the restrictions option. Webpack >= 4.44.0 requires these versions of enhanced-resolve.

sass-loader v9.0.0 started to use the restrictions option (#851, #856). This is used when the webpackImporter option is true (the default value) and a non-trivial @import is found in the sass file.

thread-loader intercepts the getResolve()() call and sends the request across the process boundary. It currently uses the vanilla JSON.stringify/JSON.parse (de)serializer, so the { restrictions: [/\.((sa|sc|c)ss)$/i] } option is turned into { restrictions: [{}] }. enhanced-resolve tries to call .test on the given object and the call fails.

Breaking Changes

Additional Info

@codecov
Copy link

codecov bot commented Sep 30, 2020

Codecov Report

Merging #102 (32198d1) into master (f3c9b7f) will increase coverage by 1.53%.
The diff coverage is 69.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   20.30%   21.84%   +1.53%     
==========================================
  Files           7        8       +1     
  Lines         458      467       +9     
  Branches       86       89       +3     
==========================================
+ Hits           93      102       +9     
  Misses        322      322              
  Partials       43       43              
Impacted Files Coverage Δ
src/WorkerPool.js 25.89% <0.00%> (ø)
src/worker.js 0.00% <0.00%> (ø)
src/serializer.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3c9b7f...32198d1. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Can you provide case where we need it?

@arcanis
Copy link

arcanis commented Jan 4, 2021

@alexander-akait We noticed our tests started breaking when thread-loader got upgraded. This is because sass-loader uses regexes (in resolve.restrictions, here), which get turned into empty objects due to the faulty serialization.

@@ -7,6 +7,7 @@ import asyncMapSeries from 'neo-async/mapSeries';

import readBuffer from './readBuffer';
import WorkerError from './WorkerError';
import { replacer, reviver } from './serializer';
Copy link

Choose a reason for hiding this comment

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

Rather than using a custom serializer, you should use the Node serialization APIs:
https://nodejs.org/api/v8.html#v8_serialization_api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer using a more familiar and portable API as long as it solves the same problem.

@alexander-akait what do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

v8 seriliazer doesn't support many feature

Copy link

Choose a reason for hiding this comment

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

@alexander-akait Just run into this PR due to function option, I'm just curious what feature is missing then JSON?

@arcanis arcanis mentioned this pull request Jan 4, 2021
3 tasks
@qnighy
Copy link
Contributor Author

qnighy commented Mar 18, 2021

Can you provide case where we need it?

I finally figured out how to reproduce it in tests, so I updated the existing test case to reflect that.

@qnighy
Copy link
Contributor Author

qnighy commented Apr 8, 2021

@alexander-akait what should I do next towards merging this PR?

@alexander-akait
Copy link
Member

@qnighy Please ping me on these weekend (busy now 😞 )

@qnighy
Copy link
Contributor Author

qnighy commented Apr 10, 2021

@alexander-akait Could you review this PR this weekend? Thanks in advance!

@alexander-akait
Copy link
Member

Hope in future we move multi threading into webpack core

@alexander-akait alexander-akait merged commit 3766560 into webpack-contrib:master Apr 10, 2021
@qnighy qnighy deleted the serialize-regexp branch April 12, 2021 05:22
bendemboski added a commit to bendemboski/embroider that referenced this pull request May 13, 2021
Update to the latest thread-loader, mainly so we can [use RegExps](webpack-contrib/thread-loader#102) in plugin options (which was release in v3.0.2).
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.

4 participants