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

Implement all path variables for webworker dynamic imports #7581

Merged
merged 1 commit into from
Jun 23, 2018

Conversation

TimHambourger
Copy link
Contributor

Previously, not all path variables were implemented when doing dynamic imports and targeting webworker. See #7563.

What kind of change does this PR introduce?

A bugfix. WebWorkerMainTemplatePlugin.js was missing some required chunk attributes.

Did you add tests for your changes?

Yes. Though I do have questions about the test framework. I'll post those as a separate comment.

Does this PR introduce a breaking change?

No. Prior to this commit, unsupported path variables would either throw at compile time or produce unexpected import paths at runtime.

What needs to be documented once your changes are merged?

No new documentation needed.

@jsf-clabot
Copy link

jsf-clabot commented Jun 22, 2018

CLA assistant check
All committers have signed the CLA.

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

@TimHambourger
Copy link
Contributor Author

As I mention above, I have a few questions about my test.

I added a test under ConfigTestCases to verify that all documented path variables compile without exception when using dynamic imports and targeting web, webworker, node, or async-node.

As is, my test just verifies that compilation doesn't throw and skips test execution with testConfig.noTests. Initially, I tried to go further and have my test confirm that the constructed import paths work as expected at runtime. This ran into friction with the test harness, mostly around supporting web and webworker targets. I'm happy to go into more details if that'd be helpful.

So mainly I'm wondering: Would you see the fuller test as valuable? If so, can we discuss ways to resolve the friction I ran into?

Thanks!

@webpack-bot
Copy link
Contributor

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

@TimHambourger
Copy link
Contributor Author

Oh, one more thought: It looks like there's an opportunity to re-use the code for formatting the dynamic import's path. I see 4 copies of it currently. It looks straightforward to add a method on MainTemplate like getDynamicImportPathExpr(chunk, chunkIdExpr) that would return a JS expr, as a string, for computing the import path from the chunkId. Usage (in WebWorkerMainTemplatePlugin) could be like

Template.ident([
    "importScript(" +
        mainTemplate.getDynamicImportPathExpr(chunk, "chunkId") +
    ");"
])

Does that seem like a good approach? If so, I'm happy to amend this PR or create a new one.

Thanks!

@webpack-bot
Copy link
Contributor

Hi @TimHambourger.

Just a little hint from a friendly bot about the best practice when submitting pull requests:

Don't submit pull request from your own master branch. It's recommended to create a feature branch for the PR.

You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR.

@sokra sokra closed this Jun 23, 2018
@sokra sokra reopened this Jun 23, 2018
@sokra sokra merged commit 3fb49de into webpack:master Jun 23, 2018
@sokra
Copy link
Member

sokra commented Jun 23, 2018

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants