sheerun / targets-webpack-plugin

Webpack plugin for transcompilig final bundles so they support legacy browsers
MIT License
15 stars 4 forks source link

Plugin breaks sourcemaps on Google Chrome #3

Closed sprguillen closed 5 years ago

sprguillen commented 5 years ago

As stated in the README: This plugin runs babel and rollup only once per asset, at the end of the compilation process.

Works like a charm as it runs my VueJS app on IE but the problem is that it breaks sourcemaps on Chrome: here's a more elaborate article on stackoverflow.

Maybe you can address this if you have plans to update the module. Thanks!

sheerun commented 5 years ago

Maybe you want to prepare and test the fix? I forked from other project where PR fixing this is available: https://github.com/simlrh/babel-webpack-plugin/pull/2/files

sprguillen commented 5 years ago

You mean I'll use babel-webpack-plugin in-conjunction with targets-webpack-plugin?

sheerun commented 5 years ago

I mean to submit PR to this repository fixing this issue. You can find fix for repository I forked from in the link I've sent before

sheerun commented 5 years ago

The issue is not with PR but with testing if it really works and that could be your part

sprguillen commented 5 years ago

I made some revisions to no avail. The PR isn't as simple as we thought it might because it requires us to use SourceMapSourceinstead of OriginalSourcewhich need us to have an inputSourceMapdefined. Adding the line where asset.sourceAndMap will be defined would lead to a Javascript heap out of memory error. I don't know if I'm doing it right since I did not author this module..

sprguillen commented 5 years ago

Reference to the changes I've applied are on this link.

sheerun commented 5 years ago

@sprguillen I've published 2.0.0-alpha.0 which should have sourcemaps support. Do you want to test it?

sprguillen commented 5 years ago

Great! Thanks, I'll gladly test it for you.

sprguillen commented 5 years ago

2018-11-15_1314

I got this error, it's quite similar after I tried to applied the fix you mentioned..

sheerun commented 5 years ago

Strange, webpack-source is in package.json so it should be installed. Maybe try to reinstall everything.

I've just published 2.0.0-alpha.1 maybe it's better. If not maybe you could experiment in code I've pushed to master and send fixing PR?

On Thu, Nov 15, 2018 at 6:16 AM Simon Phillip Guillen < notifications@github.com> wrote:

[image: 2018-11-15_1314] https://user-images.githubusercontent.com/1183103/48531770-8f539680-e8d8-11e8-944b-9bca2ea8c2c4.png

I got this error, it's quite similar after I tried to applied the fix you mentioned..

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sheerun/targets-webpack-plugin/issues/3#issuecomment-438920711, or mute the thread https://github.com/notifications/unsubscribe-auth/AAR2Dc0Wr8EPZJwo1cIPAkojslkeWHZXks5uvPiogaJpZM4Ya1NX .

sprguillen commented 5 years ago

Sorry my fault on webpack-source. I wasnt using 2.0 alpha but the Javascript heap out of memory is now happening on 2.0.0 alpha...

sheerun commented 5 years ago

Did you try 2.0.0-alpha.1 ?

sprguillen commented 5 years ago

Yes, that's the one causing the error..

sheerun commented 5 years ago

I think it's simply because sourcemaps are generated and you don't have enough memory. You can pass { sourceMaps: false } to disable generating sourcemaps but in general you should disable it for development

sheerun commented 5 years ago

Does it fail on CI as well?

sprguillen commented 5 years ago

I haven't tried on CI. We actually need sourcemaps more on development rather than in production. :) I think bundling for legacy browsers + sourcemaps overloads NodeJS.

sheerun commented 5 years ago

FYI it works for me on my CI, and on my OSX but I have 16GB ram

sprguillen commented 5 years ago

What's your current setup? I'm currently using Windows, Node 10.9.0..

I actually have 16gb ram as well.. CI means continuous integration right? I'm currently not using any CI tools. Just yarn or npm.

sheerun commented 5 years ago

Yes it means something like travis that deploys the service

On Thu, Nov 15, 2018 at 3:18 PM Simon Phillip Guillen < notifications@github.com> wrote:

What's your current setup? I'm currently using Windows, Node 10.9.0..

I actually have 16gb ram as well.. CI means continuous integration right? I'm currently not using any CI tools. Just yarn or npm.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sheerun/targets-webpack-plugin/issues/3#issuecomment-439055053, or mute the thread https://github.com/notifications/unsubscribe-auth/AAR2DVpZ8hXRfLOoUssGtoK2sendtEseks5uvXe2gaJpZM4Ya1NX .

sprguillen commented 5 years ago

I think it does make the difference, I don't use any CI tool and maybe that's why I have those heap issues. This plugin when sourceMap enabled may only be compatible with having been deployed by Travis.. Have you tried it without using Travis? Just running the node server using yarn or npm? Does it work?

sheerun commented 5 years ago

As I said it works both on my CI (I'm using server on scaleway), and on my OSX machine.

On Thu, Nov 15, 2018 at 4:18 PM Simon Phillip Guillen < notifications@github.com> wrote:

I think it does make the difference, I don't use any CI tool and maybe that's why I have those heap issues. This plugin when sourceMap enabled may only be compatible with having been deployed by Travis.. Have you tried it without using Travis? Just running the node server using yarn or npm? Does it work?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sheerun/targets-webpack-plugin/issues/3#issuecomment-439075906, or mute the thread https://github.com/notifications/unsubscribe-auth/AAR2De9ay8hHDsA3-KmkvPV9gM6wPADsks5uvYWpgaJpZM4Ya1NX .

sprguillen commented 5 years ago

We made some changes on the webpack configuration and tested on 2.0.0-alpha.1, it successfully worked! Good job! Closing this issue now..

sheerun commented 5 years ago

Glad to hear :) Mind to tell what changes?

sprguillen commented 5 years ago

We used the webpack-bundle-analyzer and restructured how plugins are being loaded by webpack, that may have reduced the amount of memory webpack is consuming when compiling code.