Remove dist_to_clean from wrangler cleanup steps

[zackbloom]

Hello! I attempted to run a preview on a project I was working on. This project has an independent webpack build, so I configured webpack to output to an index.js file in the root of the project and copied over a wrangler.toml file from the generator. When I went to preview, Wrangler deleted my dev directory, erasing any of the files I didn't have open elsewhere.

[xtuc]

Which dev directory did you use? ./worker will currently be removed I believe (that's a bug).

[zackbloom]

The project I copied the template from was called "worker", but this folder was named "afs".

…

On Sat, Jun 15 2019 at 2:39 AM, < ***@***.*** > wrote: Which dev directory did you use?./worker will currently be removed I believe (that's a bug). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub ( #255 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAANQMYH2X43THTI7FFJJT3P2S2FBANCNFSM4HYNUDYQ ).

[xtuc]

This is really strange, would we mind chatting on gchat?

[exvuma]

Is this resolved? it seemed the directory checked was af and not afs ?

[zackbloom]

You're actually seeing an artifact of my tab-completion attempt there, the folder was gone. Existing editor sessions couldn't save even after I recreated the folder.

[zackbloom]

@xtuc Sure! I'll be around tomorrow or find a time on my calendar.

[ashleygwilliams]

@zackbloom can you link to a repo that we could use to reproduce? thanks!

[zackbloom]

Yes I can!

1. Clone [email protected]:zackbloom/afs.git
2. Run npm install
3. Run wrangler preview

[EverlastingBugstopper]

Thanks @zackbloom

The issue here is that your webpack configuration specifies the output path to be the root directory. I haven't dug into the source yet, but if you change your webpack.config.js to build your worker in a dist directory, it won't delete everything. My guess is that when wrangler builds for the preview, it builds the worker and then just deletes the directory that the built javascript is in without checking what directory that is.

While we work on a fix, I'd recommend replacing your webpack config with the contents below

const path = require('path');

module.exports = {
  entry: './src/index.js',
  output: {
    path: path.resolve(__dirname, "dist"),
    filename: 'index.js'
  },
  module: {
    rules: [
      {
        test: /\.css$/i,
        use: 'raw-loader',
      },
    ],
  },
};

[EverlastingBugstopper]

@sevki Could you take a look at how we want to fix this? I'm happy to tackle a fix, I'm just not quite sure what approach we want to take here. I've confirmed that this is the issue. Wranglerjs is returning fullConfig.output.path which happens to be the development directory when the webpack configuration doesn't create a child directory.

[zackbloom]

As a user of Wrangler, I wasn't really exposed to webpack being used internally. I didn't think my webpack config would influence it's behavior at all, I just assumed I should put my output file at the location it expected.

My guess is that when wrangler builds for the preview, it builds the worker and then just deletes the directory that the built javascript is in without checking what directory that is.

Would it be possible to add a check which stopped the build if the directory already exists before it has created it?

[EverlastingBugstopper]

@zackbloom that's a good point, we should make it clearer in the docs that wrangler uses webpack.config.js to build your script if the type field in wrangler.toml is webpack. That being said, we shouldn't change the behavior as we still want to allow custom webpack builds with wrangler projects.

It does seem like the default wrangler webpack template doesn't create a webpack.config file, and you would need to create your own if you wanted to do anything requiring a specific loader for things like es6 modules. We'll likely need to discuss what the right path is here before moving forward on a fix.

[EverlastingBugstopper]

Hm, this seems like a separate bug. While wrangler generates a ./worker directory for its own build, this particular case uses a preexisting webpack.config.js that causes the entire development directory to be deleted, not just ./worker

I think this issue points to a broader problem with how wrangler handles webpack builds. We allow custom webpack config files, but in our generated projects we do not expose the webpack configuration used to build their project. I think the correct way to do this is to create a sensible default webpack config that the user can then modify/replace. This would eliminate the need to do any cleanup at all, we could leave the build artifacts where they are. This would also allow users to interact with tools like cloudworker.

[xtuc]

Got you, that's right.

This would eliminate the need to do any cleanup at all, we could leave the build artifacts where they are. This would also allow users to interact with tools like cloudworker

We cleaned the webpack artifact, not the worker ones. I doubt cloudworker will be able to take advatange of webpack's output, however the watching might.

I think the right solution is to implement #116 so that no webpack dist is only visible for the watch mode. It related to #206 because the worker output should be configurable, it's persistent already.

[EverlastingBugstopper]

Ooh, we don't clean the build artifact, got it. Is there any reason we don't just use the output specified in webpack.config.js to store the build artifacts rather than creating a separate directory? I don't think we should ask the user to configure the directory twice, and using webpack is something JavaScript devs are already familiar with. This would also (I think) eliminate the need to clean the webpack artifact. If we just used the webpack artifact itself rather than taking it and putting it in our own directory structure we wouldn't need to clean it. Does this sound right? I'm not entirely sure I understand everything going into the underlying architecture.

Edit: Perhaps it would be good to sit down and discuss

[xortive]

Some solutions I've thought of:

• don't populate dist_to_clean if the user webpack configuration provided an output folder. This implies the output folder is important as they didn't want to use the default. Easy to do.

• override any configured output folder in wranglerjs/index.js to use a folder inside the wrangler build directory (currently ./worker). Easy to do.

• replace dist_to_clean with files_to_clean, an array of filenames that are the webpack output asset files (we can get this from stats.assets in wranglerjs) and delete those individually rather than an entire directory. slightly more complex than other options but still not very difficult. I think this maintains the most compatibility with current wrangler projects as well.

• don't even do dist_to_clean. Easy to do but ugly.

[xtuc]

I would suggest doing an RFC for this issue and #206.