flyde icon indicating copy to clipboard operation
flyde copied to clipboard

Macro Editor doesn't work in local work spaces - resolving of ui bundle missed path by 1 level

Open akim-muto opened this issue 1 year ago • 12 comments

Hi, nice to meet you.

I have tried MacroEditor, but when I use flyde in VScode, MacroEditor does not work if I bundle the target tsx file with a web pack, but leave the output file in the local working directory.

If I put the file in VScode's flyde extensions directory and run it, it works.

Is there an easy way to get MacroEditor to work with the bundled files in the working directory at this time?

akim-muto avatar May 26 '24 08:05 akim-muto

Hey @akim-muto First kudos on trying to get that working! Macro UI is far from well-documented :)

Can't reproduce - see video - https://www.youtube.com/watch?v=PfogEynJ3hI

Can you share a small repro repo?

GabiGrin avatar May 27 '24 08:05 GabiGrin

@akim-muto I fixed the link to the video :)

GabiGrin avatar May 27 '24 16:05 GabiGrin

I very much appreciate your thoughtful response, which even includes a video.

I create minimal repository for reproduce macroeditor doesn’t work in local. I hope this helps you.

Here are the steps to reproduce it.

1.clone repository git clone https://github.com/project-coc/Flyde-bMacroEditor.git

2.install dependency pnpm install ※I can use bun.

3.build with webpack pnpm run build

4,you can check local macroeditor doesn’t work

5.set flyde stdlib/dist path in vscodeextinsion dir to test/test_config.json

6.install macroeditor to stdlib in vscodeextinsion pnpm run ins-vscexdir

7.restart vscode

8.can work in this case. スクリーンショット 2024-05-28 052150

Thank you!

akim-muto avatar May 27 '24 20:05 akim-muto

Thanks for the repo!

I don't know why yet, but changing editorComponentBundlePath to ../../dist/ui/Macro.js makes it work! Screenshot 2024-05-28 at 11 15 39

I found it by:

  1. reloading the extension (to make sure we're in a clean state)
  2. loading a flow in your repo
  3. opening up dev tools in VSCode
  4. could see the wrong path Screenshot 2024-05-28 at 11 15 02

Also, with the override of node library.js - I understand what you're trying to do now :) you're trying to extend the library on the right. That's something I want to allow users to do themselves. Your hack is impressive! love the resourcefulness :)

I went ahead and added supporting this as an issue here - https://github.com/flydelabs/flyde/issues/121. Is helping out and directly contributing to Flyde something that interests you?

PS: node-inst-vscdir.flyde looks wild :) assuming it was mainly to get a feeling of Flyde, but I'd inline code / new custom nodes for many things there to ensure it's saner to maintain

GabiGrin avatar May 28 '24 08:05 GabiGrin

Hi!

Thank you! I could confirm the same information here. I got lost in the use of devtool and it ate up a lot of my time lol

Also I wanted the function to add it to the node library, but the hack and node-inst-vscdir.flyde were largely a stopgap measure to see if there was a problem with the MacroEditor bundle itself, or if it was a path or dependency issue. As for node-inst-vscdir.flyde, I wanted to use the flyde functionality as you suggested.

I have a strong interest in contributing directly to Flyde. It's a tool that I hope to use for a long time if all goes well.

However, I have physical or mental health issues that make it difficult for me to guarantee a quick response or a stable response. So my direct contribution may be limited.

akim-muto avatar May 28 '24 14:05 akim-muto

Awesome 👍🏻 thanks for confirming

Thanks for the positive words and no worries, thats the beauty of contributing to open-source! It can be async, and non committing :)

On Tue, 28 May 2024 at 17:35 akim muto @.***> wrote:

Hi!

Thank you! I could confirm the same information here. I got lost in the use of devtool and it ate up a lot of my time lol

Also I wanted the function to add it to the node library, but the hack and node-inst-vscdir.flyde were largely a stopgap measure to see if there was a problem with the MacroEditor bundle itself, or if it was a path or dependency issue. As for node-inst-vscdir.flyde, I wanted to use the flyde functionality as you suggested.

I have a strong interest in contributing directly to Flyde. It's a tool that I hope to use for a long time if all goes well.

However, I have physical or mental health issues that make it difficult for me to guarantee a quick response or a stable response. So my direct contribution may be limited.

— Reply to this email directly, view it on GitHub https://github.com/flydelabs/flyde/issues/120#issuecomment-2135393326, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4N5JYMXBEYHTG4LDJ6BP3ZESI3TAVCNFSM6AAAAABIJS56COVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZVGM4TGMZSGY . You are receiving this because you commented.Message ID: @.***>

GabiGrin avatar May 28 '24 18:05 GabiGrin

Oh, I closed it becouse got a breather , but this behavior is a bug, even though it works one way or another.

I know it's not a priority, but maybe someone else will get caught up in the same thing, so do you want to manage the bug in this issue? Do you want to make a new issue?

akim-muto avatar May 29 '24 01:05 akim-muto

@akim-muto Thanks, yeah I agree it's confusing behavior. I investigated it, and this is the culprit - https://github.com/flydelabs/flyde/blob/main/resolver/src/resolver/resolve-dependencies/macro-node-to-definition.ts#L24 importPath is the full file path, including the file name, i.e. /projects/src/macro.flyde.ts, so ../dist will result in /projects/src/dist/macro.flyde.ts

It might be confusing, I agree, but wondering if this should be changed or just documented. WDYT?

GabiGrin avatar May 29 '24 06:05 GabiGrin

Hmm, I think should fix it unless it's very hard to fix. From what little I've seen, it's simply.

const editorComponentPath = join(
    importPath,
    “../”,
    macro.editorConfig.editorComponentBundlePath
);

It's not the behavior most people expect, and unfortunately all users can't be expected to read through the documentation from cover to cover when there is a problem😢

akim-muto avatar May 29 '24 11:05 akim-muto

Agree - it just means a small refactor on the stdlib and potentially breaking external existing usages

On Wed, 29 May 2024 at 14:10 akim muto @.***> wrote:

Hmm, I think should fix it unless it's very hard to fix. From what little I've seen, it's simply.

const editorComponentPath = join( importPath, “../”, macro.editorConfig.editorComponentBundlePath );

It's not the behavior most people expect, and unfortunately all users can't be expected to read through the documentation from cover to cover when there is a problem😢

— Reply to this email directly, view it on GitHub https://github.com/flydelabs/flyde/issues/120#issuecomment-2137152964, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4N5J57LPROTIS526I4U5LZEWZRNAVCNFSM6AAAAABIJS56COVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZXGE2TEOJWGQ . You are receiving this because you commented.Message ID: @.***>

GabiGrin avatar May 29 '24 12:05 GabiGrin

Since we're at it, I'll try PR if there are no problems with the code changes. It might take some time to prepare the environment for development flyde and to prepare for confirm behavior. It's a feature that is not yet well documented, and Do you think it doesn't needs to be done so urgently?

I'll also try to document the process of preparing the environment, which will be the windows 10 version.

akim-muto avatar May 29 '24 13:05 akim-muto

@akim-muto awesome! it should be pretty straightforward - clone, pnpm i and pnpm start You can ping me at discord as well if you're stuck and need a faster answer https://www.flyde.dev/discord/

Regarding the docs, are you asking whether I think this should be documented? or fixed?

Documentation - yes, for sure, there's #88 open for that. Moreover, there's the structured editor for Macros that isn't documented at all - see example - https://github.com/flydelabs/flyde/blob/main/stdlib/src/Lists/Lists.flyde.ts#L88

I see both documenting Macros and fixing this low priority. But given the fix here is really simple (the code you've mention + a quick find and replace on existing stdlib libs), it's a good first PR

GabiGrin avatar May 29 '24 17:05 GabiGrin