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

resolving of ui bundle missed path by 1 level #120 #124

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

akim-muto
Copy link
Contributor

New breaking change here

  • Who does this affect:
    Their are using Macro Editor.

  • How to migrate:
    Change way to specify path to Macro Editor bundle path.
    Directory one up.

   ditorComponentBundlePath: "../../../macro.tsx" → ditorComponentBundlePath: "../../macro.tsx"

  • Why make this breaking change:
    We changed it to the one that most people expect as a behavior.

  • Severity (number of people affected x effort):
    Undocumented and few users are expected.
    The response is simply to replace the letters.
    Therefore, the impact is low.

Copy link

vercel bot commented Jun 6, 2024

Someone is attempting to deploy a commit to the Flyde Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Jun 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
flyde-playground ⬜️ Ignored (Inspect) Visit Preview Jun 13, 2024 10:31am

@GabiGrin GabiGrin self-requested a review June 9, 2024 05:58
Copy link
Contributor

@GabiGrin GabiGrin left a comment

Choose a reason for hiding this comment

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

@akim-muto great job!
One problem though - I just checked it on a test project, and because it has a local copy of the stdlib (because it installed
@flyde/runtime) it didn't work - the refactor of stdlib you've done did not reach it. A bit context here - The extension will always use a local copy of the stdlib if you've installed it, and only if it can resolve a @flyde/stdlib package in the working dir, it will use its own copy. After your change, everyone who installed @flyde/runtime will be stuck until they upgrade it, which is hard to enforce.

This means this is a breaking change - meaning there is no way around having temporary backwards compact support - in case the path doesn't exist, we can check the old path - without ".." - and leave a comment explaining why and suggest it can be removed in the future

@akim-muto
Copy link
Contributor Author

Response to review.
Thanks for your time.

@@ -21,11 +21,20 @@ export function macroNodeToDefinition<T>(
editorComponentBundleContent: "",
} as MacroEditorConfigCustomDefinition,
};
const editorComponentPath = join(
importPath,
const BundlePathPlusOne = join(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is redundant, you can inline it in line 30
as in just:

let editorComponentPath = join(
    importPath,
    '..',
    macro.editorConfig.editorComponentBundlePath
);

    

@GabiGrin
Copy link
Contributor

@akim-muto reviewed again, this should work - left some improvement comments for the code itself

@akim-muto
Copy link
Contributor Author

akim-muto commented Jun 12, 2024

Oh, you meant to leave it in the code comments.
I thought need to put out a Warning.

I'm going!

@GabiGrin GabiGrin self-requested a review June 13, 2024 10:31
@GabiGrin
Copy link
Contributor

@akim-muto great job, tested and backwards compat. is working great 👍

@GabiGrin GabiGrin merged commit e5fddd1 into flydelabs:main Jun 13, 2024
3 checks passed
@akim-muto akim-muto deleted the resolving-bundle-path-i120 branch June 13, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants