Skip to content
This repository has been archived by the owner on May 10, 2021. It is now read-only.

track NoN files when configured dirs are used, clean before each new run #134

Merged
merged 2 commits into from
Jan 14, 2021

Conversation

lindsaylevine
Copy link
Contributor

@lindsaylevine lindsaylevine commented Jan 8, 2021

this is a pass at tracking NoN-moved or NoN-generated files between runs of NoN and cleaning them before a new run.

i don't see a way we can add a "manual" clean option for plugin CLI users. for example, if they build on the CLI four consecutive times, this change should allow them to do that without having to change our file overwrite logic at all. BUT, when they're done building/deploying on the CLI, they will be left with all the NoN files in their .git, as @FinnWoelm pointed out in the original issue that has since been transferred internally. because the plugin will only run on netlify build, i don't think adding a "clean" flag/command to NoN will make a difference; there'd be no way for the plugin to access it outside of a build. these users will have to stash or manually delete these files after finishing their builds/deploys. open to ideas here. this may be a greater CLI issue.

to-do:

  • test with plugin
  • write cleanup script

@lindsaylevine lindsaylevine added the type: bug code to address defects in shipped code label Jan 8, 2021
@lindsaylevine
Copy link
Contributor Author

addresses netlify/next-runtime#74

@lindsaylevine lindsaylevine changed the title track NoN files when configured dirs are used and clean before running NoN track NoN files when configured dirs are used, clean before each new run Jan 8, 2021
@lindsaylevine lindsaylevine force-pushed the ll/clean-custom-dirs branch 2 times, most recently from 44ae8af to c468b53 Compare January 8, 2021 07:36
@lindsaylevine lindsaylevine marked this pull request as draft January 8, 2021 08:39
@lindsaylevine
Copy link
Contributor Author

lindsaylevine commented Jan 8, 2021

currently this doesnt work in these ways:

  1. if node_modules or node_modules/.cache gets purged between cli builds
    • only way around this right now would be to purge your NoN stuff from your configured dirs; not great
  2. as mentioned in the PR desc, NoN files are tracked by .git unless they're deleted after cli builds
    • somehow find a "clean up" option via the plugin
    • could possibly find a way to clean after a deploy? but if they're just "build"ing and not deploying then that would only be a bandaid

@FinnWoelm
Copy link
Collaborator

Awesome!! 😊

Your approach is interesting! Initially, when I thought about this feature, I imagined that we could a custom copySync function, that tracks each added file, since (almost?) our logic uses copySync, e.g. Netlify Function setup and Public files setup. copySync could just be a wrapper for fs-extra.copySync(), but extend that functionality with also writing the target file into the cache (or pushing it into an array that gets written to file at the end, although then we need to handle interruptions/crashes that occur during next-on-netlify build — an issue that we don't have when we write to the cache file right away).

But this approach is really neat!! It seems like one downside of this approach vs copySync wrapper is that we might mistakenly tag some files as NoN if they are manually put into the functions folder while NoN is running. But that's an edge case. Downside of copySync wrapper is, of course, that it requires extra work/custom wrapper for each IO operation, such as writeFile, etc... (as we already do for _redirects).

My personal thoughts regarding

  1. Loss of cache when purging node_modules: I think we can live with that. Seems like it'll probably be a rare edge case.
  2. Clean up option: Would be good to add, I think. For now, this could just be part of next-on-netlify and plugin users could install next-on-netlify if they want/need to take advantage of a dedicated cleanup function. But we could also add this in a second step, IMO. Doesn't necessarily need to be part of this PR :)

@lindsaylevine lindsaylevine marked this pull request as ready for review January 13, 2021 08:58
@lindsaylevine
Copy link
Contributor Author

@FinnWoelm issue with your cleanup script idea. we purposely fail the plugin if anyone has next-on-netlify manually installed in their project (https://github.com/netlify/netlify-plugin-nextjs/blob/main/index.js#L42). we could change this, but after playing around with writing a custom script, it seems like an unpleasant bandaid that will need urgent replacing.

@lindsaylevine
Copy link
Contributor Author

lindsaylevine commented Jan 13, 2021

i will be starting conversations internally about improving the CLI experience because i do believe now this is a greater CLI issue and one that could impact other plugins. i think the CLI shouldn't be making direct changes to a project's directory. the entire "build" should run in/be output into a tmp directory to avoid messing with a project directly.

Copy link
Contributor

@cassidoo cassidoo left a comment

Choose a reason for hiding this comment

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

This seems like a clever approach!! :)

@lindsaylevine lindsaylevine merged commit 2371424 into main Jan 14, 2021
@lindsaylevine lindsaylevine deleted the ll/clean-custom-dirs branch January 14, 2021 07:06
lindsaylevine added a commit that referenced this pull request Jan 14, 2021
- Fix: fallback blocking would cause builds to crash ([#139](#139))
- next/image initial support ([#138](#138))
- generate headers file to override static chunks cache control ([#141](#141))
- track NoN files for configured dirs to clean before each run ([#134](#134))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants