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 for multiple config files (when compiling typescript config files) #509

Closed

Conversation

ltetzlaff
Copy link

@ltetzlaff ltetzlaff commented May 28, 2019

What did you implement:

Closes #439, #490

How did you implement it:

If .stats is an Array check for errors in all compilations.
I got errors twice on the way thus used the last output for these.

How can we verify it:

I use a webpack.config.ts extending another webpack.config.ts.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: HELL YEAH
Is it a breaking change?: NO

@ltetzlaff ltetzlaff changed the title Multiple config files Fix for multiple config files May 28, 2019
@ltetzlaff ltetzlaff changed the title Fix for multiple config files Fix for multiple config files (when compiling typescript config files) May 28, 2019
@hassankhan
Copy link
Contributor

Hi @ltetzlaff, thank you so much for your PR 😄! Sorry you had to deal with the ESLint failures, we had an issue where branch protection checks were being bypassed.

I had a quick look at 25db8de which is where you made the actual change, I believe (do let me know if I'm wrong 👍). On the surface, the change seems great - it would be awesome if you could add a few tests checking/verifying the new behaviour, along with an update to the README.md describing how users can take advantage of this feature.

@ltetzlaff
Copy link
Author

ltetzlaff commented Jun 6, 2019

Sorry you had to deal with the ESLint failures, we had an issue where branch protection checks were being bypassed.

No worries, I guessed there was just something out of place from a previous PR or so ✌️

add a few tests checking/verifying the new behaviour

Hmm I took a brief look and honestly can't seem to find a good place to insert a test because existing tests seem to throw in intermediate objects of the compilation pipeline. Where do you think should I put an example webpack.config.ts and pack from it? Or would just wrapping one of these intermediate objects in an array in an additional .stats property as in 25db8de#diff-71a1f6779f4a9a1fe69fcec81f85db12R39 suffice?

@RishikeshDarandale
Copy link

@hassankhan , @ltetzlaff , any update on this?

@RishikeshDarandale
Copy link

@ltetzlaff , Can you please rebase and get ready this one for merge?

@hassankhan , please let us know when can be this included in release.

@HyperBrain
Copy link
Member

HyperBrain commented Aug 19, 2019 via email

@RishikeshDarandale
Copy link

@HyperBrain, any luck?

Copy link
Member

@HyperBrain HyperBrain left a comment

Choose a reason for hiding this comment

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

LGTM

@HyperBrain HyperBrain added this to the 5.3.2 milestone Aug 27, 2019
@HyperBrain
Copy link
Member

@RishikeshDarandale @ltetzlaff Finally, I'm preparing the latest update. Due to the other fixes, there are some conflicts in some of the files now. Can you try to fix them, so that I can get this one in? For the package.json, it should be up to date already in master.

@ltetzlaff
Copy link
Author

Hi there, sorry for leaving this one barren for so long - will take a look at rebasing it onto master in the next days, feel free to do so before if you need it for the release or so ✌️🔜

@RishikeshDarandale
Copy link

@ltetzlaff , looks like this change will not work correctly. if package.individually is set to true, plugin is creating an multi-compile option for each individual function.

Thus, according to me if the multi-compile option is provided as below, then package.individually should not be allowed/permitted. When multi-compile option is provided, then it;s the responsibility of client to create the package as per one the need.

module.exports = [
  {
    entry: ./src/server.js',
    target: 'node',
    externals: [nodeExternals()],
    output: {
      libraryTarget: 'commonjs',
      path: path.join(__dirname, '.webpack'),
      filename: '[name].js'
    },
  },
  {
    entry: './src/client.js',
    target: 'node',
    output: {
      libraryTarget: 'commonjs',
      path: path.join(__dirname, '.webpack'),
      filename: '[name].js'
    },
  },
];

@hassankhan / @HyperBrain , let me know your thoughts as well.

I have made these changes in validate.js for the above one. I have to still verify all permutation and combination if this multi-compile change.

@RishikeshDarandale
Copy link

@HyperBrain , @hassankhan , @ltetzlaff I have created the new pull request #537 with my understanding. Please verify and let me know your comments!

@miguel-a-calles-mba miguel-a-calles-mba modified the milestones: 5.3.2, 5.3.3 Apr 22, 2020
@miguel-a-calles-mba miguel-a-calles-mba requested a review from a team May 6, 2020 15:02
@miguel-a-calles-mba
Copy link
Member

Is this PR still needed? Or do we use #537 instead?

@ltetzlaff
Copy link
Author

I'm closing this in favor of #537, looks better to me ✌️

@ltetzlaff ltetzlaff closed this May 6, 2020
@miguel-a-calles-mba miguel-a-calles-mba removed this from the 5.3.3 milestone May 6, 2020
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.

Errors when using a Webpack config with multiple configurations
5 participants