Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spago does not emit an error when bundling an app's main module that does not have a main function #181

Open
JordanMartinez opened this issue May 3, 2019 · 18 comments

Comments

@JordanMartinez
Copy link
Contributor

Steps to reproduce:

mkdir explore-bundle-no-main
cd explore-bundle-no-main
spago init
# edit the `src/Main.purs` file to remove the "main" function
spago bundle -m Main -t dist/main.js

Spago doesn't fail with an error that Main.purs does not have a function called main. Thus, the output of the bundle is this:

// Generated by purs bundle 0.12.5
var PS = {};
PS["Main"].main();

One won't realize that the source of their issue is the compiled output until after exploring a few things more.

I only came across this because I used the wrong module name in the spago bundle command.

@JordanMartinez JordanMartinez changed the title Spago bundles a module without a main function Spago does not emit an error when bundling an app's main module that it does not have a main function May 3, 2019
@JordanMartinez JordanMartinez changed the title Spago does not emit an error when bundling an app's main module that it does not have a main function Spago does not emit an error when bundling an app's main module that does not have a main function May 3, 2019
@f-f
Copy link
Member

f-f commented May 4, 2019

Good catch! This is interesting because spago bundle just shells out to purs bundle, which is happy to generate that malformed js.

Pulp however implements this check, and fails with a nice error message:

$ pulp build -m Main -t dist/main.js
* Building project in /Users/fabrizio/code/explore-bundle-no-main
* Build successful.
* Bundling JavaScript...
* Main cannot be used as an entry point module because it
* does not export a `main` value.
*
* If you need to create a JavaScript bundle without an entry point, use
* the --skip-entry-point flag.
*
* ERROR: Failed entry point check for module Main

Since that looks like a bunch of code, this makes me wonder if it should be instead purs to implement the check instead.
We could implement it here, but the compiler is a heavy dependency that we've been trying to avoid by using the externs files as pulp does

EDIT: it looks like there's an entire PureScript package dedicated to this check: externs-check

@f-f f-f added bug RFC and removed bug labels May 4, 2019
@f-f
Copy link
Member

f-f commented May 4, 2019

Ping @hdgarrood: thoughts on adding this check to purs?

@hdgarrood
Copy link
Contributor

This is something we previously decided is best not done inside purs bundle (since purs bundle only has access to the generated js). See purescript/purescript#2086

@f-f
Copy link
Member

f-f commented May 4, 2019

@hdgarrood thanks for the context! It makes sense, I'll look into using the externs files for the check 👍

@f-f f-f added bug and removed RFC labels May 4, 2019
@hdgarrood
Copy link
Contributor

Then again I suppose since purs bundle is unlikely to be run against any JS files which aren’t output from purs compile, perhaps it would make more sense to just point purs bundle at the compiler’s output directory, so that it would have the information it needed to be able to implement this check inside purs.

@hdgarrood
Copy link
Contributor

It is certainly less than ideal that pulp is depending on the externs files format, since these are not part of the compiler’s public API.

@f-f
Copy link
Member

f-f commented May 4, 2019

Indeed, but if it's necessary to rely on them and other tools (e.g. spago) start doing that too then it could make sense to include them in the public API? (though I'd be more for purs checking on this by itself)

@hdgarrood
Copy link
Contributor

Yeah what I’m suggesting is that we modify purs bundle to have access to externs files as well as the js so that it can do this check.

@hdgarrood
Copy link
Contributor

I created an issue: purescript/purescript#3621

@f-f
Copy link
Member

f-f commented May 5, 2019

Thank you!

@f-f
Copy link
Member

f-f commented Mar 29, 2020

I'm removing the "blocked" label because a good first take at this (i.e. solving 90% of the problem) would be literally grepping the file searching for the string "main ="

@Cmdv
Copy link

Cmdv commented Sep 24, 2020

Assuming this isn't been worked, I'd be interested in picking it up 😄

I've not worked on spago before so wondered if I could be pointed to where I'd do the searching for the string "main ="?

@samhh
Copy link
Contributor

samhh commented Jun 25, 2021

What's an updated repro for this? Or am I being dense in failing to follow the original repro? 😄

@f-f
Copy link
Member

f-f commented Jun 25, 2021

@samhh bundle was replaced by bundle-app in the meantime. The updated repro is:

mkdir explore-bundle-no-main
cd explore-bundle-no-main
spago init
echo "module Main where" > src/Main.purs
spago bundle-app
node index.js

The last command fails because there's no main function to be run.

@f-f
Copy link
Member

f-f commented Jun 25, 2021

@Cmdv I'm so sorry I missed your comment! Every once in a while some notification slips through, apologies. Also feel free to ping me in the future.

The patch should go somewhere in this function, somewhere before line 379, which is where we call the compiler:

spago/src/Spago/Build.hs

Lines 376 to 381 in 8b8f06a

bundleApp withMain maybeModuleName maybeTargetPath noBuild buildOpts usePsa =
let (moduleName, targetPath) = prepareBundleDefaults maybeModuleName maybeTargetPath
bundleAction = Purs.bundle withMain (withSourceMap buildOpts) moduleName targetPath
in case noBuild of
DoBuild -> Run.withBuildEnv usePsa buildOpts $ build (Just bundleAction)
NoBuild -> Run.getEnv >>= (flip runRIO) bundleAction

If we are bundling withMain (which is the only occurrence where we need to worry about this. Actually I'm not sure why this is parametrized, bundle-app should always run WithMain) then we should read the module that we're bundling, and to a text search for main =.

At this point though I'm thinking that it might be easier to fix this in the compiler.

@f-f
Copy link
Member

f-f commented Sep 29, 2023

This is even easier these days - we'd just need to copy this bit of code

@Cmdv
Copy link

Cmdv commented Sep 29, 2023

I'm so sorry I missed your comment! Every once in a while some notification slips through, apologies.

@f-f well looks like I'm even worse just spotted your reply only took me 2 years 😅

@f-f
Copy link
Member

f-f commented Sep 29, 2023

It's alright, this issue has been open for 4y, we'll eventually get to it 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants