AngelMunoz / Finny

A cross-platform tool for unbundled front-end development that doesn't depend on Node or requires you to install a complex toolchain
https://perla-docs.web.app/
MIT License
132 stars 11 forks source link

Perla 1 beta feedback #96

Closed nojaf closed 1 year ago

nojaf commented 1 year ago

Hi, I'm trying out the Perla beta. My code is at https://github.com/nojaf/telplin/pull/23.

Here is some feedback in no particular order:

App.js and logo.png are not being served. So I can't really get everything to work.

That is what I got so far. I might have some more feedback once I've gotten a little further.

AngelMunoz commented 1 year ago

I can no longer do dotnet perla s, I miss that shorthand.

I'll add that one back

I see [08:49:49 INF] Now listening on: https://127.0.0.1:8900 in the logs, but for the SLL certificate to work that needs to be https://localhost:8900.

You can change the host in the devServer.host node in the perla.json configuration file however I will indeed change that default to local host it makes sense

I'm unable to figure the mountDirectories thing out:

I went ahead and check your code, I did the following changes to perla.json and perla.json.importmap and it should be running now

{
  "$schema": "https://raw.githubusercontent.com/AngelMunoz/Perla/dev/perla.schema.json",
  "index": "./index.html",
  "fable": {
    "project": "./OnlineTool.fsproj",
    "extension": ".js"
  },
  "mountDirectories": {
    "/": ".",
    "/fable_modules": "./fable_modules"
  },
  "devServer": {
    "port": 8900,
    "useSSL": true
  },
  "envPath": "/telplin/env.js",
  "dependencies": [
    {
      "name": "@monaco-editor/react",
      "version": "4.4.6"
    },
    {
      "name": "lz-string",
      "version": "1.5.0"
    },
    {
      "name": "react",
      "version": "17.0.1"
    },
    {
      "name": "react-dom",
      "version": "17.0.1"
    },
    {
      "name": "use-sync-external-store",
      "version": "1.2.0"
    }
  ],
  "devDependencies": []
}
{
  "imports": {
    "@monaco-editor/loader": "https://ga.jspm.io/npm:@monaco-editor/loader@1.3.2/lib/es/index.js",
    "@monaco-editor/react": "https://ga.jspm.io/npm:@monaco-editor/react@4.4.6/lib/es/index.js",
    "lz-string": "https://esm.sh/lz-string@1.5.0",
    "object-assign": "https://ga.jspm.io/npm:object-assign@4.1.1/index.js",
    "prop-types": "https://ga.jspm.io/npm:prop-types@15.8.1/index.js",
    "react": "https://ga.jspm.io/npm:react@17.0.1/index.js",
    "react-dom": "https://ga.jspm.io/npm:react-dom@17.0.1/index.js",
    "scheduler": "https://ga.jspm.io/npm:scheduler@0.20.2/index.js",
    "state-local": "https://ga.jspm.io/npm:state-local@1.0.7/lib/es/state-local.js",
    "use-sync-external-store": "https://ga.jspm.io/npm:use-sync-external-store@1.2.0/index.js",
    "use-sync-external-store/shim": "https://ga.jspm.io/npm:use-sync-external-store@1.2.0/shim/index.js"
  }
}

App.js and logo.png are not being served. So I can't really get everything to work.

Images are not being served for some reason... I need to check that one out

The dependencies notation feels like a step back: { "name": "react-dom", "version": "18.2.0" } could have been { "react-dom": "18.2.0" }.

I will consider this one, thanks for the feedback

You will be able to try v 1.0.0-beta-010 once it is out and indexed by nuget with some of this feedback as well

nojaf commented 1 year ago

Thanks a bunch for the quick response here!

nojaf commented 1 year ago

Alright, next batch of remarks. I'm still not out of the woods for Telplin. (Note that I moved the source a little bit https://github.com/nojaf/telplin/tree/main/tool/client)

importSideEffects StyleSheet


After build, I get both an `App.css` and `oneline-tool.css` file in my dist folder. I suppose the `online-tool.css` file is a copy of a file in the mounted folder but it should end up in the dist folder.
- Due to my config, `perla.json` and `perla.json.importmap` are also copied to the `dist` folder. I'm aware this is due to my way of working but nonetheless, those files should be ignored.
- I'm not sure how useful `[11:24:22 INF] HTTP GET /fable_modules/fable-library.4.0.1/String.js responded 200 in 0.0769 ms` the logs are in the default mode. Feel more like a `DEBUG` message to me.

Overall, it does feel a bit that there is no end-to-end test. 
It would be nice if, for the Fable case, a sample application was built, launched with a static file server and have some sort of UI test for it.
AngelMunoz commented 1 year ago

Current during perla serve logo.png is not served

Indeed I'm still working to figure out this one

fable_modules is not copied to the dist folder after a perla build. This is quite problematic as it is required to run the application

After build, I get both an App.css and oneline-tool.css file in my dist folder. I suppose the online-tool.css file is a copy of a file in the mounted folder but it should end up in the dist folder.

When you run build, esbuild takes care of bundling everything it needs into the single file that includes css bits, that's why you also get App.js and App.css at the end but your configuration poses some problems for this (continued below)

Due to my config, perla.json and perla.json.importmap are also copied to the dist folder. I'm aware this is due to my way of working but nonetheless, those files should be ignored.

Technically it is not a supported scenario even for v0, I'm not sure how it was working from the start, but it is interesting indeed!

Configuration files and source files should live in a different directory. I would suggest switching to an actually supported project directory like the one below.

.tool
  assets // images or other static content
    favicon.ico
    logo.png
  src // source files (however images and other static content can also live here)
    OnlineTool.fsproj
    App.fs
    App.js
    // the rest...
  perla.json
  perla.json.importmap
  index.html

It is briefly mentioned in the docs but I guess we can make it more clear and add a warning when we find a non-standard directory.

fable_modules is not copied to the dist folder after a perla build. This is quite problematic as it is required to run the application

I think is the same issue we had with the index.html being overwritten by the source files, since you're mounting the current directory and we copy mounted directories at the end (excluding F# files) we're most likely overwriting the already compiled minified and bundled esbuild output with the source files.

I'm not sure how useful [11:24:22 INF] HTTP GET /fable_modules/fable-library.4.0.1/String.js responded 200 in 0.0769 ms the logs are in the default mode. Feel more like a DEBUG message to me.

I did think of this myself but I had situations where I was not sure if a file was being served, indeed it is more a debug message, I think we'll hide it behind a flag.

Overall, it does feel a bit that there is no end-to-end test.

Do you mean testing app code or perla code? (one is in the works, the other is well... it could be better 😅)

It would be nice if, for the Fable case, a sample application was built, launched with a static file server and have some sort of UI test for it.

Oh yes, a preview command would be nice, it just builds the app and launches a static server in the output directory I think this could be able to make it into v1.

nojaf commented 1 year ago

esbuild takes care of bundling everything it needs into the single file

I'm afraid that is not the case. Even after playing ball and dropping everything in the src folder. The first line of dist/App.js is import{Record as w,Union as M}from"./fable_modules/fable-library.4.0.1/Types.js";. There is no bundling happening. Which I actually prefer, but in order for this to work the fable_modules folder needs to be copied as well.

AngelMunoz commented 1 year ago

I Just pushed some of the feedback changes to beta 12, and If you don't mind I'll check telplin's code again and make a PR with the src directory just to be it is a config issue and not a hidden bug somewhere

nojaf commented 1 year ago

Thanks, I've also noticed that swapping the order of physical files / virtual files in the configuration is preventing the following to work:

{
  "mountDirectories": {
    "/": "./src",
    "/": "./assets"
  }
}

I cannot add key "/" twice so I cannot mount two folders as if they were one and the same.

AngelMunoz commented 1 year ago

After helping you a bit with the telplin project's docs I have to say that you have chosen interesting setups, in all honesty I never intended supporting duplicated keys or mounting src on root (which I found incredibly weird that it worked in the v0.x versions)

Mounting files on root has always been meant for files that actually have to live in the root of the server (like service workers to support PWAs which require it to be in the root of the server) but not necessarily to mount the whole application in root, as I was following established conventions in similar tools in the ecosystem.

Mounting src on root is a use case I do intend to support as it makes sense in certain scenarios.

However, I won't support the following scenario:

In regard of mounting multiple directories in the same path... I will have to give it a more time to think about it, as it is prone to conflicts when it comes to source content IMO, also it invites the user to add a bunch of glob files that include/exclude content which can complicate their setups when there are already solutions to this.

In your example above there are two things that can be done

The first one will make perla behave as the expected use case above, the second one means having a different url for these assets, but those changes are minimal and not invasive at the same time I don't think that holds back anyone from working.

Besides that, my main concern in the multiple directories per url is how is one supposed to deal with duplicated content e.g.

{
  "mountedDirectories": {
    "/": "./src",
    "/": "./docs"
  }
}
src
  README.md
  app.js
docs
  README.md
  Index.md

What happens to the README files? which one gets copied? What happens when the src one is not meant to be copied? we'd likely have to add an "exclude" glob.

One of the goals in perla is to leave the configuration file as untouched as possible by following the existing conventions it should "just workâ„¢"

I need to be more explicit about these in the documentation.

nojaf commented 1 year ago

Taking a step back, what is the rationale behind having a mountDirectories configuration in the first place? Perhaps there is a misunderstanding about what problem it solves.

AngelMunoz commented 1 year ago

Tools like vite can build a resource/dependency graph from the main module in index.html so they don't really need things like mountDirectories we don't have such nice tools so we can offer to mimic those local paths in the server.

That means the rationale was to map local paths to the server paths, conveniently we can actually map one local path to a different server path but that's more like an unintended side-effect.

The only case I had in mind to map a directory to the root was to enable service workers but the rest of the cases where one would want to map a certain app (instead of a single file or set of loose files) completely flew over my head.

AngelMunoz commented 1 year ago

I guess I'm short sighted in some aspects of how people can use some of the features, but I think making source files (not the root of the project) being able to be mounted anywhere in the server paths might be a good use case to support.

You were not doing super weird stuff and mounting things in root is going to be supported it was just an oversight on my part thanks for this second batch of invaluable feedback!

nojaf commented 1 year ago

That means the rationale was to map local paths to the server paths, conveniently we can actually map one local path to a different server path but that's more like an unintended side-effect.

Perhaps mountDirectories should then just be a list of folders. As in these are mounted. Although I think in practice you would rather want to have an exclude list for this instead.

And we could introduce a public folder concept if you really need a file to be on the root.

Tools like vite can build a resource/dependency graph from the main module in index.html

For my understanding, what stops us from doing this? The processing of the content of the module?

AngelMunoz commented 1 year ago

Perhaps mountDirectories should then just be a list of folders. As in these are mounted. Although I think in practice you would rather want to have an exclude list for this instead.

I thought about this, but I felt it could become very inflexible rather quick especially now that I know what can be tried and done with the mappings we have today.

And we could introduce a public folder concept if you really need a file to be on the root.

That seems useful but for things like service workers sometimes you might want to tap into your existing sources (e.g re-using models or logic that works in both workers and the main window), I think we can leave mappings as it is but we need to introduce a way to have "loose entries" so they can be processed by esbuild as well but as a separate bundle this will be useful to introduce multi-page-applications eventually

For my understanding, what stops us from doing this? The processing of the content of the module?

For the simplest use case we'd need a javascript parser so we can extract imports (and maybe dynamic imports) from the source content, but given that I also want to support what esbuild supports I'd be missing parsers for jsx/tsx/typescript

In the case of Javascript I recently found https://github.com/sebastienros/esprima-dotnet but I haven't dive into it a lot

AngelMunoz commented 1 year ago

Feedback from this issue has been taken to the according issues and is being tracked individually.

Once again thanks for your feedback and taking the time to test out perla beta versions hopefully once I have this in better shape it will be simpler to use even for "opinionated" setups and we can bring you back. Right now I expect myself to focus on staying in the "conventions" setup and once we have better foundations we can explore more flexible use cases.