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

feat: unpack bootstrap using TypeScript for cross-platform compatibility #5

Closed
wants to merge 1 commit into from

Conversation

HRAshton
Copy link

@HRAshton HRAshton commented Jun 17, 2024

No description provided.

@tmokmss
Copy link
Owner

tmokmss commented Jun 17, 2024

Hi @HRAshton, thanks! I was not aware of this platform issue. I assumed these commands are executed inside the docker container, but yeah, the underlying NodejsFunction does local bundling if esbuild is available locally, and it may not work on different shell or Windows environment.

So, another solution is to use forceDockerBundling flag, which forces bundling commands run in a container and become free from cross-platform issues. What do you think?

I'm afraid your solution may not work after npm publish because ts-node or unzipper are in dev dependencies. Also I'd like to keep dependencies as small as possible for maintainability.

@HRAshton
Copy link
Author

Hmm.. Make sense, I'll try it, thank you

@HRAshton HRAshton closed this Jun 17, 2024
@tmokmss
Copy link
Owner

tmokmss commented Jun 17, 2024

I guess something like this should work on Windows.

    new LlrtFunction(this, 'Handler', {
      entry: 'handler.ts',
      bundling: {
        forceDockerBundling: true,
      },
    });

Please let me know if it doesn't work. I don't have Windows environment in hand 🙏 Then we can update README to describe the workaround.

edit) I confirmed it works on my spare win machine :)

@tmokmss
Copy link
Owner

tmokmss commented Jun 17, 2024

@HRAshton I updated the code to automatically set forceDockerBundling: true on Windows platform. b9e41f7 along with updating README. Let me know if you have further problem. Thanks again!

@HRAshton
Copy link
Author

Yes, I just checked it out, works great) Thanks!

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