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 custom webpackChunkName for aggressive import #552

Merged
merged 2 commits into from
Jun 5, 2020

Conversation

kaysonwu
Copy link
Contributor

fix custom webpackChunkName for aggressive import

Summary

babel-plugin will ignore the correct custom webpackChunkName during dynamic import. I think it makes sense to keep this, it can store chunks in the specified directory.

Test plan

error

success

@kaysonwu
Copy link
Contributor Author

@theKashey

@theKashey
Copy link
Collaborator

Your PR looks good, but I honestly don't know why loadable tries to manage all magic comments.
So @gregberge

@kaysonwu
Copy link
Contributor Author

Firstly, it can be kept backward compatible, and secondly it is consistent with code

@kaysonwu
Copy link
Contributor Author

When can this PR be processed? @theKashey

@theKashey
Copy link
Collaborator

PR is ok, but if you are really want to use this feature without breaking in the future I urge you to add a non-snapshot based test as in the example above.
So this PR could be processed after adding just one line.

@kaysonwu
Copy link
Contributor Author

@theKashey Non-snapshot test? Add to that file? babel-plugin It's all snapshot tests

@kaysonwu
Copy link
Contributor Author

@theKashey Can you describe it in more detail?

@kaysonwu
Copy link
Contributor Author

@theKashey The enhanced test feels redundant to me because the snapshot is clear enough. However, I followed your advice and enhanced the key position test.

@theKashey
Copy link
Collaborator

Snapshot is big, detailed, and not self explanatory. As well as fragily. How many times I did just -u, and missed a little, but so important change.

A small check after snapshot is pointing on what is REALLY IMPORTANT.

Copy link
Collaborator

@theKashey theKashey left a comment

Choose a reason for hiding this comment

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

@gregberge - approving this PR, but the merge is on you.

const result = testPlugin(`
loadable(props => import(/* webpackChunkName: "pages/[request]" */ \`./pages/\${props.path}\`))
`)

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add here expect(result).toEqual(expect.stringContaining('/* webpackChunkName: "pages/[request]"'')) to as long as snapshot based tests are hard to track things. Ie you cannot infer the expected behavior from the test itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kaysonwu - this advice.

function getChunkNamePrefix(chunkName) {
if (typeof chunkName !== 'string') return ''
const match = chunkName.match(/(.+?)\[(request|index)\]$/)
return match ? match[1] : ''
}

function replaceChunkName(callPath) {
const agressiveImport = isAgressiveImport(callPath)
const values = getExistingChunkNameComment(callPath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

look like webpackChunkName is the only thing thing function returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The returned webpackChunkName is not complete, so I think it should be the prefix


addOrReplaceChunkNameComment(callPath, { webpackChunkName })
addOrReplaceChunkNameComment(callPath, { ...values, webpackChunkName })
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to spread values, as long as there is only webpackChunkName inside.
Let's better get rid of them

let { webpackChunkName } = getExistingChunkNameComment(callPath) || {};

or, even better, make getExistingChunkNameComment return a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let { webpackChunkName } = getExistingChunkNameComment(callPath) || {};

The above writing will lead to the loss of other magic comments.

@kaysonwu
Copy link
Contributor Author

I feel a bit frustrated because it is not an actively maintained project.

@kaysonwu
Copy link
Contributor Author

@gregberge Can you give a clear answer about this PR?

@theKashey
Copy link
Collaborator

@kaysonwu - my clean answer is "I've approved it". However, as long as only @gregberge is able to actually release the update we have to wait for him in any case.

@kaysonwu
Copy link
Contributor Author

@theKashey your answer is clear, thank you very much! But @gregberge didn't deal with it in time. Unknown waiting is the most painful

@theKashey
Copy link
Collaborator

Please understand - it's always a balance between "real work", "stuff" you might do, "family", and OSS is eating a piece of everything above.
Sometimes there is no time, or just power left for non life crucial things.

@kaysonwu
Copy link
Contributor Author

kaysonwu commented Apr 24, 2020

@theKashey I can see that @gregberge is active on github, but he doesn't deal with PR.

@theKashey theKashey merged commit e4a7574 into gregberge:master Jun 5, 2020
@theKashey
Copy link
Collaborator

Yep, so I'll merge your PR with only my approval.

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.

2 participants