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

Jump to bottom

Remove type: 'module' from new Worker exprs #12750

Merged
merged 3 commits into from Mar 12, 2021
Merged

Remove type: 'module' from new Worker exprs #12750

merged 3 commits into from Mar 12, 2021

Conversation

RReverser
Copy link
Contributor

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

No. The issue is, in Node.js (which Jest uses) everything works fine even without this change, since type: 'module' is silently ignored, and so I'm not sure how to test it aside from manually verifying the change.

It looks like there are no snapshots to compare against either.

For example, the output of test/configCases/worker/** tests has changed, and I could see it manually, but tests pass either way.

Does this PR introduce a breaking change?

Potentially - in addition to existing restrictions, it will now also skip expressions like new Worker(new URL('some.js'), import.meta.url), someDynamicExpressionForOptionsObject()) whereas previously they would be accepted. I think this is a correct change though, and unlikely to hit anyone in practice.

What needs to be documented once your changes are merged?

N/A

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@RReverser RReverser changed the title Remove type: 'module from new Worker exprs Remove type: 'module' from new Worker exprs Feb 22, 2021
@RReverser
Copy link
Contributor Author

Note: I've also dedented the large if (url.isString()) { ... } block by folding into if (!url || !url.isString()) return;, so it's best to view diff with "ignore whitespace changes" option.

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra
Copy link
Member

sokra commented Feb 22, 2021 •
edited

Great. For the tests: it's not jest that's handling workers. For testing we add a custom worker impl located here: https://github.com/webpack/webpack/blob/master/test/helpers/createFakeWorker.js

@RReverser
Copy link
Contributor Author

@sokra Ah, okay, let me try that.

@RReverser
Copy link
Contributor Author

@sokra Made the necessary change and verified that the test succeeds with my changes but fails when I remove delete options.type.

@RReverser
Copy link
Contributor Author

@sokra Is the test change looking okay / anything else to add?

@RReverser
Copy link
Contributor Author

@sokra Ping - could you please review?

@RReverser RReverser closed this Mar 10, 2021
@RReverser RReverser reopened this Mar 10, 2021
@RReverser
Copy link
Contributor Author

@alexander-akait I see you liked the comment - if you want to review, that's also welcome 😀 This is literally our only blocker left...

@alexander-akait
Copy link
Member

@RReverser Yep, thanks, don't close, maybe we need improve some more cases

@RReverser
Copy link
Contributor Author

Yep, thanks, don't close, maybe we need improve some more cases

Oh, I closed and reopened only because CLA check was stuck and @sokra suggested that as a solution. Other than that it should be ready to go.

@RReverser
Copy link
Contributor Author

It's just that I'll be making a talk in the end of March that will be relying on a library that I can OSS only after this issue is resolved, and I'm getting a bit worried about this dependency chain and don't know what else to do on my side 😀 (meanwhile, I can point npm to my own fork for testing, but it won't be that helpful for other consumers of the said lib)

@sokra
Copy link
Member

sokra commented Mar 11, 2021

I'll finish that today or tomorrow. Sorry for the delay.

@sokra sokra force-pushed the remove-worker-type-module branch from 29d62a8 to 61cc65c Compare March 12, 2021 11:09
@sokra sokra merged commit e643b85 into webpack:master Mar 12, 2021
@sokra
Copy link
Member

sokra commented Mar 12, 2021

Thanks

@RReverser
Copy link
Contributor Author

Thank you!!

@RReverser RReverser deleted the remove-worker-type-module branch March 12, 2021 14:06
@dependabot dependabot bot mentioned this pull request Mar 14, 2021
Merged
Brian-McBride added a commit to AnatidaeProject/webpack that referenced this pull request Mar 14, 2021
@Brian-McBride
Squashed commit of the following:
6c19c68 
commit 58dfda2
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Sun Mar 14 20:33:03 2021 +0100

    5.25.1

commit 3f2a269
Merge: e643b85 69218d4
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Sun Mar 14 20:32:00 2021 +0100

    Merge pull request webpack#12891 from webpack/bugfix/non-js-entry

    fix problem with startup of non-js initial chunks

commit 69218d4
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Sun Mar 14 19:54:34 2021 +0100

    fix problem with startup of non-js initial chunks

    fixes webpack#12880

commit e643b85
Merge: be352e8 61cc65c
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Fri Mar 12 14:47:23 2021 +0100

    Merge pull request webpack#12750 from RReverser/remove-worker-type-module

commit 61cc65c
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Fri Mar 12 12:09:02 2021 +0100

    improve parsing to handle non-literal options

commit e839494
Author: Ingvar Stepanyan <rreverser@google.com>
Date:   Mon Feb 22 17:43:38 2021 +0000

    Disallow type:module + importScripts in tests

commit ea2cdeb
Author: Ingvar Stepanyan <rreverser@google.com>
Date:   Mon Feb 22 16:25:24 2021 +0000

    Remove `type: 'module` from `new Worker` exprs

    Fixes webpack#12686.

commit be352e8
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Fri Mar 12 08:49:03 2021 +0100

    5.25.0

commit ab74839
Merge: bcf3cb2 c014665
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Thu Mar 11 21:00:05 2021 +0100

    Merge pull request webpack#12871 from webpack/feature/generate-asset

    add `emit` option for asset modules

commit bcf3cb2
Merge: 8ea0a6a 7245527
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Thu Mar 11 20:34:48 2021 +0100

    Merge pull request webpack#12872 from webpack/bugfix/ignore-asset-modules

    allow to define "ignored modules" per dependency

commit c014665
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Thu Mar 11 19:31:33 2021 +0100

    update cli snapshots

commit 7572217
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Thu Mar 11 18:44:41 2021 +0100

    add `emit` option for asset modules

    fixes webpack#12474

commit 7245527
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Thu Mar 11 15:04:53 2021 +0100

    allow to define "ignored modules" per dependency

    new URL() will use `"data:"` when ignore

commit 85a6eee
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Thu Mar 11 16:41:19 2021 +0100

    move parser.filename back to generator.filename
This was referenced Mar 14, 2021
Merged
Merged
This was referenced Mar 15, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
PR: CI-ok PR: unreviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@RReverser @webpack-bot @sokra @alexander-akait