Move from eslint-loader to eslint-webpack-plugin, close #847 #985
Conversation
|
Hey @Kocal! Thanks for jumping on this. Biggest concerns are about the BC stuff. Could we keep
I'm ok with that. I'm also not sure about the test failure unfortunately :/ |
|
I've recreated the pull request, For the failing tests, the funny thing is that they pass when running them one by one... 😬 Something wrong must happens before them. |
|
Okay I got it. The issues were caused by my weird trick which check if an ESLint configuration exists or not (don't ask me why lol): try {
(async function() {
await eslint.calculateConfigForFile('webpack.config.js');
})();
} catch (e) {
// ...
}It looks like I can't do that netheir use the form So I had three options:
WDYT? |
|
Hmmm. So normally, it's impossible to use an async function in Encore, because we need Encore to run synchronously (we can't wait for some async process to finish before returning the Webpack config via But, in your case, this is just to trigger a warning/error, right? It seems to me that we could run that async, sort if "in the background". If I'm picturing everything correctly, the result would be that Webpack builds... then a short time after, an error is thrown. But I'm doing some guessing here :). |
Considering webpack supports a Promise for configuration, we can make
Yeah it's for triggering a warning/error, but also to add the ESLintPlugin to the webpack plugins. We need to run that sync otherwise the plugin won't be added (or added later, after webpack started to run... 🤔). Also, since the "node.js packages" tends to be async more and more, I'm afraid this will cause more and more issues to Encore. WDYT about:
|
That's only true if the user does not customize the generated config before returning it. So that would still require a semver-major. Also, this will likely break the |
|
Oh yeah, totally forgot about this... 😥 Well... idk. |
|
Yep - we can try rpc I was going to say the same thing as Stof, but he already said it better. I think we can still do it (though, I haven’t thought about the reset part), but it would need to be opt in until the next major version (with a deprecation of that flash is not set). |
|
Just my opinion, but for me, in a new major release, it would be really nice if we stop exporting For the async const Encore = require('@symfony/webpack-encore');
Encore.addEntry(...);
const config = await Encore.getWebpackConfig();
// do something with config
module.exports = config; |
|
@Kocal top-level await works only in ES modules, not in CommonJS files. |
|
Ah yes that's true... |
|
Hi guys 👋 Is there anything else to do here? Using Thanks! |
There was a problem hiding this comment.
Sorry for the long delay @Kocal - could you rebase this so we can see some green tests (the tests on main are passing).
|
The PR has been successfully rebased, and a deprecation log has been added! |
|
(friendly ping @weaverryan 🙂 ) |
|
Looking forward for this PR to be merged :) |
|
Thank you @Kocal! |
|
And thank you for always being a support and helper around the Symfony JS ecosystem @Kocal! My apologies for this taking SO long to get merged. |
|
@weaverryan no worries, I know you are super busy! :) and thanks for your words ❤️ |
|
@Kocal i think this documentation is outdated and should be updated https://symfony.com/doc/current/frontend/encore/advanced-config.html#having-the-full-control-on-loaders-rules can you do this please? |
Hi 👋
This PR is a proposal for #847 which add the eslint-webpack-plugin, since the eslint-loader is deprecated.
There are a lot of changes:
BC method.enableEslintLoader()has been removed, in favor of.enableEslintPlugin(). It accepts an object or a function..enableEslintLoader()has not been removed, so it's not a BC anymorelintVuewas too specific and not futur proof (if you want to lint GraphQL, JSON or whatever you want, with ESLint, see last § of Proposal to replace #504 (ESLint/Vue) #574 (comment)). Extensions can easily be configured throughEncore.enableEslintPlugin(options => { options.extensions.push('vue'); })eslint.CLIEngineis not used anymore to detect if an ESLint configuration exists, because this class is deprecated since ESLint 7.However, some tests are failing but I don't know why:The dependency@babel/plugin-syntax-dynamic-importis present in Encore'spackage.json, so I don't really understand the issue... 😬Any help would be appreciated! ❤️EDIT: fixed, see #985 (comment)
WDYT? Thanks!