noseglid / atom-build

:hammer: Build your project directly from the Atom editor
https://atom.io/packages/build
MIT License
248 stars 97 forks source link

Refresh targets ignores .atom-build.json #527

Open wiggisser opened 6 years ago

wiggisser commented 6 years ago

When I add a new target to my .atom-build.json and then do "Packages -> Build -> refresh targets" the new target does not appear in the list of available targets. I have to restart atom to have those changes reflected. Same for removing a target or updating its cmd

noseglid commented 6 years ago

Atom build is actively watching the build file so a refresh target shouldn't be necessary. That it doesn't even work when you're issuing "refresh targets" (which is a manual way of doing what should be automatic) is weird.

Are you getting any errors in the console or something like that?

Might add that I am unable to reproduce, so there's likely something else at play here.

wiggisser commented 6 years ago

I did open the developer tools (Ctrl+Shift+I) and ran a refresh targets. Did not show any additional message at all. I set a breakpoint in the refreshtargets function and the breakpoint got hit shortly after I saved changes to the file, so the watch mechanism seems to work.

I then tried to dig deeper and came to the point where the actual json file is loaded, the function getConfig(file) in atom-build.js. And this seems to be the problem: Whereas in my file in the editor I only have two targets defined, the require returns an object which contains 4 targets. It seems, this always returns the inital state of the file, when it was first required at the atom start and does not reread the file after it was changed.

I'm currently running atom 1.20 on linux mint, but I noticed this issue on all versions of atom since I created this issue, and also on Windows 8.1

wiggisser commented 6 years ago

I replaced require with readfile and parse for JSON file. This works for me, and refreshes the targets.

I'm pretty sure, this issue applies for .js files too, but I didn't touch anything there. See also this stackoverflow thread on require and its cache https://stackoverflow.com/questions/9210542/node-js-require-cache-possible-to-invalidate

Sorry, didn't see, you already do a delete on the require cache, but that doesn't seem to work (at least for json files, did not try js)

scratchboom commented 6 years ago

As a workaround, I just rename .atom-build.json to .atom-build.cson. json and cson are compatible (at least for my simple .atom-build file)

jmariner commented 6 years ago

I investigated this problem since I had a similar issue with a package I'm writing. It turns out that Atom uses its own require cache and clearing the normal require.cache does not affect the custom one.

The custom require cache is at snapshotResult.customRequire.cache (snapshotResult is a global). The paths within are all relative to [atom installation dir]/app-x.xx.x/resources/app.asar/static/, so I used the following to build this path and remove it from the cache:

const path = require("path");
const originalPath = "absolute/path/to/file";

const cachePath = path.relative(
    process.resourcesPath + 
        // these directory names don't actually matter, could be "/a/b" instead
        "/app.asar/static",
    originalPath
).replace(/\\/g, "/"); // custom cache forces "/" as the path separator

delete snapshotResult.customRequire.cache[cachePath];

This would probably be the better approach to fixing this instead of manually reading and parsing the files, but may break in the future. Ideally, Atom needs to implement a concrete way to clear the require cache since it's very common for packages like this one to need to reload required JS or JSON files.

Edit: I just found that this was somewhat discovered (https://github.com/noseglid/atom-build/pull/542#issuecomment-355219340), but no full fix was created. I can turn this into a PR to fix this if needed.

wiggisser commented 6 years ago

@jmariner Thanks for your comment. I already figured this out quite a while ago (see the comments in #542 for details) long story short: atom won't change the current behavior. Thus all packages requiring module reload need either an alternative loading mechanism (for instance like in #553) or force a restart of atom.