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

Prevent an error when the first argument of import is a template #4511

Closed
wants to merge 2 commits into from

Conversation

fatfisz
Copy link
Contributor

@fatfisz fatfisz commented Jun 1, 2018

Fixes one of the problems described in #4433.

The handle-import plugin can't deal with webpack's dynamic imports. Before this change an unhelpful error was appearing in the console and the build was broken. The change ignores such imports altogether, resulting in no error.

Handling template literals by Next.js can be added later on, but maybe it would be better done through a webpack plugin instead of a babel one.

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Should this actually throw a helpful error then 🤔

Besides that, it looks good 🙏

@fatfisz
Copy link
Contributor Author

fatfisz commented Jun 7, 2018

For now I'd appreciate it if there was no error - otherwise I'd need to disable the plugin completely. Next.js will work with this almost fine - the chunks just won't be marked as used on the server side. This seems to be an obscure scenario anyway, so maybe adding a note that glob imports are not fully supported yet (but won't break the build) will suffice?

I'll continue working on patching Webpack in an even better way so that maybe in the future the Babel plugin can be replaced completely with a Webpack one.

@timneutkens
Copy link
Member

What I'm not following is why'd you want to make something that doesn't function work without compilation errors 🤔 Wouldn't that make it even worse for people that don't know it doesn't work use it 🤔

@fatfisz
Copy link
Contributor Author

fatfisz commented Jun 8, 2018

Ok, fair point. While I made it work somehow and it almost works out of the box, it might not be a good user experience. I'll disable the plugin for now and maybe come back to this later (though in a Webpack plugins incarnation).

@fatfisz fatfisz closed this Jun 8, 2018
@morgs32
Copy link

morgs32 commented Jun 21, 2018

@fatfisz you break ground on the webpack plugin? Interested in helping.

@fatfisz
Copy link
Contributor Author

fatfisz commented Jun 22, 2018

I've actually managed to do what I needed, you can explore the results here: https://github.com/fatfisz/burdoc/tree/master/src/BurdocWebpackPlugin.

In a nutshell, I disable the handle-imports Next.js Babel plugin entirely, and instead I patch Webpack internals.

It goes something like this:

@lock lock bot locked as resolved and limited conversation to collaborators Jun 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants