Refuse to overwrite existing target bundle file unless it was generat…

…ed by Spago in the first place (purescript#1260)

CHANGELOG.md

@@ -19,6 +19,9 @@ Other improvements:
 - Added typo suggestions upon failing to find a package by name.
 - `spago publish` now checks that the publish location matches one of the remotes in the current Git repository.
 - Emoji ✅ ❌ ‼️ replaced with ✓ ✘ ‼ respectively, and are not printed at all with `--no-color`.
+- `spago bundle` now writes a special marker into the bundle and will refuse to
+  overwrite the file if the marker isn't present, assuming that the file was
+  manually created or edited, not generated by Spago itself.
 
 ## [0.21.0] - 2023-05-04
 

bin/src/Flags.purs

@@ -69,9 +69,16 @@ outfile =
   OT.optional
     $ O.strOption
         ( O.long "outfile"
-            <> O.help "Destination path for the bundle"
+            <> O.help "Destination path for the bundle. Defaults to ./index.js"
         )
 
+forceBundle :: Parser Boolean
+forceBundle =
+  O.switch
+    ( O.long "force"
+        <> O.help "Overwrite the target bundle file even if it was manually created or generated by a tool other than Spago."
+    )
+
 -- TODO make an ADT for node and browser
 platform :: Parser (Maybe String)
 platform =

bin/src/Main.purs

@@ -172,6 +172,7 @@ type BundleArgs =
   , backendArgs :: List String
   , bundlerArgs :: List String
   , output :: Maybe String
+  , force :: Boolean
   , pedanticPackages :: Boolean
   , type :: Maybe String
   , ensureRanges :: Boolean
@@ -406,6 +407,7 @@ bundleArgsParser =
     , backendArgs: Flags.backendArgs
     , bundlerArgs: Flags.bundlerArgs
     , output: Flags.output
+    , force: Flags.forceBundle
     , pedanticPackages: Flags.pedanticPackages
     , ensureRanges: Flags.ensureRanges
     , strict: Flags.strict
@@ -757,8 +759,17 @@ mkBundleEnv bundleArgs = do
         let cliArgs = Array.fromFoldable bundleArgs.bundlerArgs
         (Alternative.guard (Array.length cliArgs > 0) *> pure cliArgs) <|> (bundleConf _.extraArgs)
 
-  let bundleOptions = { minify, module: entrypoint, outfile, platform, type: bundleType, sourceMaps: bundleArgs.sourceMaps, extraArgs }
   let
+    bundleOptions =
+      { minify
+      , module: entrypoint
+      , outfile
+      , force: bundleArgs.force
+      , platform
+      , type: bundleType
+      , sourceMaps: bundleArgs.sourceMaps
+      , extraArgs
+      }
     newWorkspace = workspace
       { buildOptions
           { output = bundleArgs.output <|> workspace.buildOptions.output

src/Spago/Command/Bundle.purs

@@ -2,10 +2,15 @@ module Spago.Command.Bundle where
 
 import Spago.Prelude
 
+import Data.Array (all, fold, take)
+import Data.String as Str
+import Data.String.Utils (startsWith)
 import Node.Path as Path
 import Spago.Cmd as Cmd
 import Spago.Config (BundlePlatform(..), BundleType(..), Workspace, WorkspacePackage)
 import Spago.Esbuild (Esbuild)
+import Spago.FS as FS
+import Spago.Generated.BuildInfo as BuildInfo
 
 type BundleEnv a =
   { esbuild :: Esbuild
@@ -21,6 +26,7 @@ type BundleOptions =
   , sourceMaps :: Boolean
   , module :: String
   , outfile :: FilePath
+  , force :: Boolean
   , platform :: BundlePlatform
   , type :: BundleType
   , extraArgs :: Array String
@@ -35,7 +41,7 @@ type RawBundleOptions =
   , extraArgs :: Array String
   }
 
-run :: forall a. Spago (BundleEnv a) Unit
+run :: ∀ a. Spago (BundleEnv a) Unit
 run = do
   { esbuild, selected, workspace, bundleOptions: opts } <- ask
   logDebug $ "Bundle options: " <> show opts
@@ -47,38 +53,104 @@ run = do
       BundleBrowser, BundleApp -> "--format=iife"
       _, _ -> "--format=esm"
 
-    -- See https://github.com/evanw/esbuild/issues/1921
-    nodePatch = case opts.platform of
-      BundleNode -> [ "--banner:js=import __module from \'module\';import __path from \'path\';import __url from \'url\';const require = __module.createRequire(import.meta.url);const __dirname = __path.dirname(__url.fileURLToPath(import.meta.url));const __filename=new URL(import.meta.url).pathname" ]
-      _ -> []
+    onlyForNode s = case opts.platform of
+      BundleNode -> s
+      BundleBrowser -> ""
 
-    output = case workspace.buildOptions.output of
-      Nothing -> "output"
-      Just o -> o
+    output = workspace.buildOptions.output # fromMaybe "output"
     -- TODO: we might need to use `Path.relative selected.path output` instead of just output there
     mainPath = withForwardSlashes $ Path.concat [ output, opts.module, "index.js" ]
 
-    shebang = case opts.platform of
-      BundleNode -> "#!/usr/bin/env node\n\n"
-      _ -> ""
-
     { input, entrypoint } = case opts.type of
-      BundleApp -> { entrypoint: [], input: Cmd.StdinWrite (shebang <> "import { main } from './" <> mainPath <> "'; main();") }
-      BundleModule -> { entrypoint: [ mainPath ], input: Cmd.StdinNewPipe }
+      BundleApp ->
+        { entrypoint: []
+        , input: Cmd.StdinWrite $ fold [ onlyForNode "#!/usr/bin/env node\n\n", "import { main } from './", mainPath, "';main();" ]
+        }
+      BundleModule ->
+        { entrypoint: [ mainPath ]
+        , input: Cmd.StdinNewPipe
+        }
+
     execOptions = Cmd.defaultExecOptions { pipeStdin = input }
 
-    args =
-      [ "--bundle"
-      , "--outfile=" <> outfile
-      , "--platform=" <> show opts.platform
-      -- See https://github.com/evanw/esbuild/issues/1051
-      , "--loader:.node=file"
-      , format
-      ] <> opts.extraArgs <> minify <> sourceMap <> entrypoint <> nodePatch
+    banner = fold
+      [ bundleWatermarkPrefix
+      , BuildInfo.packages."spago-bin"
+      , " */"
+      , onlyForNode nodeTargetPolyfill
+      ]
+
+    args = fold
+      [ [ "--bundle"
+        , "--outfile=" <> outfile
+        , "--platform=" <> show opts.platform
+        , "--banner:js=" <> banner
+        , "--loader:.node=file" -- See https://github.com/evanw/esbuild/issues/1051
+        , format
+        ]
+      , opts.extraArgs
+      , minify
+      , sourceMap
+      , entrypoint
+      ]
+
+  -- FIXME: remove this after 2024-12-01
+  whenM (FS.exists checkWatermarkMarkerFileName)
+    $ unless opts.force
+    $ whenM (isNotSpagoGeneratedFile outfile)
+    $ die [ "Target file " <> opts.outfile <> " was not previously generated by Spago. Use --force to overwrite anyway." ]
+
   logInfo "Bundling..."
   logDebug $ "Running esbuild: " <> show args
   Cmd.exec esbuild.cmd args execOptions >>= case _ of
     Right _ -> logSuccess "Bundle succeeded."
     Left r -> do
       logDebug $ Cmd.printExecResult r
       die [ "Failed to bundle." ]
+
+isNotSpagoGeneratedFile :: ∀ a. String -> Spago (BundleEnv a) Boolean
+isNotSpagoGeneratedFile path = do
+  exists <- FS.exists path
+  if not exists then
+    pure false
+  else
+    -- The first line of the file could be the marker, or it could the shebang
+    -- if the bundle was compiled for Node, in which case the marker will be the
+    -- second line. So we check the first two lines.
+    FS.readTextFile path
+      <#> Str.split (Str.Pattern "\n")
+      >>> take 2
+      >>> all (not startsWith bundleWatermarkPrefix)
+
+bundleWatermarkPrefix :: String
+bundleWatermarkPrefix = "/* Generated by Spago v"
+
+-- Presence of this file gates the watermark check.
+--
+-- If this file exists in the current directory, the Bundle command will check
+-- if the target bundle file already exists and has the watermark in it, and if
+-- it doesn't have the watermark, will refuse to overwrite it for fear of
+-- overwriting a user-generated file.
+--
+-- We gate this check on the presence of this file so that the check is only
+-- performed in a controlled context (such as integration tests), but doesn't
+-- work for normal users. The idea is that the users who upgrade their Spago
+-- aren't immediately met with a refusal to overwrite the bundle. Instead, Spago
+-- will overwrite the bundle just fine, but now the bundle will have the
+-- watermark in it. Then, after some time, after enough users have upgraded and
+-- acquired the watermark in their bundles, we will remove this gating
+-- mechanism, and watermark checking will start working for normal users.
+checkWatermarkMarkerFileName :: String
+checkWatermarkMarkerFileName = ".check-bundle-watermark"
+
+-- A polyfill inserted when building for Node to work around this esbuild issue:
+-- https://github.com/evanw/esbuild/issues/1921
+nodeTargetPolyfill :: String
+nodeTargetPolyfill = Str.joinWith ";"
+  [ "import __module from 'module'"
+  , "import __path from 'path'"
+  , "import __url from 'url'"
+  , "const require = __module.createRequire(import.meta.url)"
+  , "const __dirname = __path.dirname(__url.fileURLToPath(import.meta.url))"
+  , "const __filename=new URL(import.meta.url).pathname"
+  ]

test-fixtures/bundle-app-browser.js

@@ -1,3 +1,4 @@
+/* Generated by Spago v0 */
 (() => {
   // output/Effect.Console/foreign.js
   var log = function(s) {

test-fixtures/bundle-app-node.js

@@ -1,5 +1,5 @@
 #!/usr/bin/env node
-import __module from 'module';import __path from 'path';import __url from 'url';const require = __module.createRequire(import.meta.url);const __dirname = __path.dirname(__url.fileURLToPath(import.meta.url));const __filename=new URL(import.meta.url).pathname
+/* Generated by Spago v0 */import __module from 'module';import __path from 'path';import __url from 'url';const require = __module.createRequire(import.meta.url);const __dirname = __path.dirname(__url.fileURLToPath(import.meta.url));const __filename=new URL(import.meta.url).pathname
 
 // output/Effect.Console/foreign.js
 var log = function(s) {

test-fixtures/bundle-default.js

@@ -0,0 +1,15 @@
+/* Generated by Spago v0 */
+(() => {
+  // output/Effect.Console/foreign.js
+  var log = function(s) {
+    return function() {
+      console.log(s);
+    };
+  };
+
+  // output/Main/index.js
+  var main = /* @__PURE__ */ log("\u{1F35D}");
+
+  // <stdin>
+  main();
+})();

test-fixtures/bundle-module.js

@@ -1,3 +1,5 @@
+/* Generated by Spago v0 */
+
 // output/Effect.Console/foreign.js
 var log = function(s) {
   return function() {

test-fixtures/bundle-refuse-overwrite-output.txt

@@ -0,0 +1,14 @@
+Reading Spago workspace configuration...
+
+✓ Selecting package to build: project
+
+Downloading dependencies...
+Building...
+           Src   Lib   All
+Warnings     0     0     0
+Errors       0     0     0
+
+✓ Build succeeded.
+
+
+✘ Target file index.js was not previously generated by Spago. Use --force to overwrite anyway.

test/Prelude.purs

@@ -1,6 +1,6 @@
 module Test.Prelude
-  ( module X
-  , module Test.Prelude
+  ( module Test.Prelude
+  , module X
   ) where
 
 import Spago.Prelude
@@ -107,17 +107,20 @@ shouldEqualStr v1 v2 =
       ]
 
 checkFixture :: String -> String -> Aff Unit
-checkFixture filepath fixturePath = do
-  filecontent <- String.trim <$> FS.readTextFile filepath
+checkFixture filepath fixturePath = checkFixture' filepath fixturePath (shouldEqualStr `on` String.trim)
+
+checkFixture' :: String -> String -> (String -> String -> Aff Unit) -> Aff Unit
+checkFixture' filepath fixturePath assertEqual = do
+  filecontent <- FS.readTextFile filepath
   overwriteSpecFile <- liftEffect $ map isJust $ Process.lookupEnv "SPAGO_TEST_ACCEPT"
   if overwriteSpecFile then do
     Console.log $ "Overwriting fixture at path: " <> fixturePath
     let parentDir = dirname fixturePath
     unlessM (FS.exists parentDir) $ FS.mkdirp parentDir
-    FS.writeTextFile fixturePath (filecontent <> "\n")
+    FS.writeTextFile fixturePath (String.trim filecontent <> "\n")
   else do
-    expected <- String.trim <$> FS.readTextFile fixturePath
-    filecontent `shouldEqualStr` expected
+    expected <- FS.readTextFile fixturePath
+    filecontent `assertEqual` expected
 
 plusDependencies :: Array String -> Config -> Config
 plusDependencies deps config = config