Atomic copyFile to foreign.js breaks filewatchers

[milesfrain]

Description

Filewatchers of bundlers like parcel and webpack break when their inputs are atomically overwritten. purs does this for foreign.js files, and so automatic rebundling is not triggered upon changes in FFI .js code.

To Reproduce

A repo with steps to reproduce this issue is available here:
https://github.com/milesfrain/ffi-reload

Expected behavior

Ideally output/**/foreign.js files would be written in the same way as output/**/index.js files, with writeTextFile:

purescript/src/Language/PureScript/Make/Actions.hs

Line 225 in 1d78a36

writeTextFile jsFile (TE.encodeUtf8 $ js <> mapRef)

instead of atomic copyFile:

purescript/src/Language/PureScript/Make/Monad.hs

Line 186 in 1d78a36

Directory.copyFile src dest

While it is less efficient to read-then-write these files, the performance hit should be negligible with these small FFI files, and it seems like a worthwhile tradeoff to improve interoperability with the web development ecosystem.

Additional context

@blonkhart (on slack) shared a workaround for parcel using an updated chokidar dependency:

// package.json
  "resolutions": {
    "parcel/**/chokidar": "2.1.3"
  }

PureScript version

0.13.6

[blankhart]

A few additional thoughts for what they're worth:

• For parcel, the chokidar resolution is actually to an earlier version (current is v3+). When using this approach yarn emits a warning that this will break with Node v14+. For the source see this parcel issue. Update: This does not actually fix the parcel HMR issue. If spago is running alongside in watch mode, parcel will reload when an FFI file is changed, but does not update the bundle to reflect the changes to the FFI file. So there does not seem to be a good workaround at present.

• For webpack and maybe rollup, users encountering this issue potentially can work around it by configuring the bundler to watch the output directory rather than watching for change notifications from the file system (I haven't tried this with either).

I'd view this request as an enhancement rather than a bug as purs makes a choice that is efficient and correct, and the breakage seems to depend on the file watch behavior of a bundler. I agree that the enhancement is worthwhile so that there is no need to change bundler workflows, especially given PureScript's emphasis on seamless FFI. Thank you @milesfrain for posting the issue.

[hdgarrood]

Thanks for writing this up. I'm going to remove the "bug" label because I agree that this isn't a bug, but it's still worth considering. Also, I think it's worth noting that although FFI files are usually small, we can't assume that they always are. A data point: the largest FFI file in the official package set currently is Data.HashMap/foreign.js at 33K, which I agree is not large enough to worry about. However I wouldn't be surprised if there's some proprietary projects which generate enormous FFI files using tools for whatever reason.

[afcondon]

I have to feel that somehow FFI files shouldn't be really huge - that there should be a best practice that tends to limit it somehow. Can't offer any solid evidence for this tho'

Basically if an FFI file is huge it must have some very large JS functions in it or a huge number of FFI endpoints (or both). In the former case, it seems like maybe the bulk of the JS could live somewhere else (ie installed by npm or something) and in the latter, seems like it'd be undesirable too, so many signatures to maintain, huge surface area.

So, it would be nice to hear of any proprietary projects that have needed this, and hear the rationale for it.

[hdgarrood]

Eh, I would say "FFI files should not be larger than x KB" is a constraint that is reasonable to impose within a style guide, but not reasonable to impose within the actual compiler.

[afcondon]

Agreed.

[blankhart]

Nontrivial work does occur in FFI files, but a convenient, fast edit/refresh cycle also takes on increased importance in those cases because JavaScript doesn't provide PureScript's guarantees and mistakes only show up when you run the thing.

If large files produced by tooling are a real concern, in principle the compiler could impose some size-based, documented cutoff and safe-write those on the theory that the user is less likely to depend on file watchers detecting when they change. It seems to me though that even in this case a user is more likely to be surprised and frustrated by the failure of a watcher in that case than slower/unsafer duplication of the file.

[milesfrain]

This might be undesirable feature creep, but a CLI flag to enable safe-write could meet the needs of all users.

Also, I initially started writing this as a proposal, but was swayed by the "If this doesn’t look right, choose a different type" message and the bug template which seemed to fit better.

[hdgarrood]

Yeah, I would prefer to copy "unsafely" all of the time rather than make this configurable via a CLI flag. No worries re labelling - I agree that the bug template fits this issue better, I just wanted to explain why I removed the label.

[hdgarrood]

We just observed a bug due to not writing normal JS output files atomically in Slack. We saw this appearing in the output:

// output/Kunden.AuftragEditor/index.js
module.exports = {
    Initialize: Initialize,
    Finalize: Finalize,
    <...omitted for brevity...>
    berufBestandteilComponent: berufBestandteilComponent,
    component: component,
    newtypeForm: newtypeForm,
    newtypeBerufForm: newtypeBerufForm,
    functorBerufQuery: functorBerufQuery,
    newtypeBerufBestandteilForm: newtypeBerufBestandteilForm
};
fBestandteilComponent: berufBestandteilComponent,
    component: component,
    newtypeForm: newtypeForm,
    newtypeBerufForm: newtypeBerufForm,
    functorBerufQuery: functorBerufQuery,
    newtypeBerufBestandteilForm: newtypeBerufBestandteilForm
};

Using atomic writes (i.e. writing the file to a temporary directory and then only moving it to its desired destination when it’s complete) would have avoided this. Then again, this particular issue was probably caused by having two purs processes running concurrently on the same output directory, which is something we should probably disallow anyway, via a simple file lock.

[hdgarrood]

Given that bugs arising from non-atomic copies do pop up in practice, I'm not really very keen on changing this. Ideally we would be moving in the other direction, i.e. making more of the filesystem operations the compiler performs atomic.

An example of a bug arising from non-atomic copies which I often run into is in GHC, where it's common to see errors along the lines of:

cannot find object file ‘.stack-work/dist/x86_64-linux-tinfo6/Cabal-3.0.1.0/build/Module.dyn_o’
while linking an interpreted expression

although I believe this is now fixed in newer versions of GHC.

I'm actually a bit bewildered that webpack and parcel ask you so shamelessly to turn off this functionality in your editors; it exists for a very good reason. In my mind it's definitely webpack and parcel which ought to be fixed here, not purs.

[milesfrain]

In my mind it's definitely webpack and parcel which ought to be fixed here

I agree. I'm hoping snowpack doesn't have this issue, so that when ES modules output is available, we'll have a better iterative development option.