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

Changing from aws-chromelambda to @sparticuz/chromium #492

Conversation

thiagodmont
Copy link
Contributor

Following the discussion on this issue.

Change the library to use @sparticuz/chromium
Needed to make some small implementations.
Since dockerLambda is deprecated I changed the way that we test the lambda.

Please feel free to guide me if it doesn't suit the requirements.

"start": "docker run -d -p 8080:8080 -t example-renderer-aws-lambda:latest"
},
"dependencies": {
"@loki/renderer-aws-lambda": "file:../../packages/renderer-aws-lambda",
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of doing this, add this project to the workspace by adding it to workspaces.packages in the root package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this way to be able to iterate during the development and have the dependencies updated inside node_modules when I installed them. If I use the version when I install this it will fetch from the npm and not from the code I changed.

Is there another way to achieve this?

I added in the other comment why I didn't use the workspace.

Copy link
Owner

Choose a reason for hiding this comment

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

There's an option for yarn workspaces called nohoist which I think should solve this problem: https://classic.yarnpkg.com/blog/2018/02/15/nohoist/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I did the change using workspace and nohoist.

package.json Outdated
@@ -10,7 +10,8 @@
"scripts": {
"format": "prettier '{,packages/**/,examples/**/,website/**/,docs/**/,test/**/,.github/**/}*.{md,js,jsx,json,yml}' --write",
"lint": "eslint packages/*/src",
"test": "jest",
"prepare:lambda:test": "cd examples/running-renderer-aws-lambda && yarn build && yarn start",
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of cd ... use yarn workspace @loki/example-running-renderer-aws-lambda run build once you've fixed the workspace thing. Also maybe name this pretest so you can get rid of it in the line below.

Copy link
Contributor Author

@thiagodmont thiagodmont Nov 30, 2023

Choose a reason for hiding this comment

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

Ok, I'm going to change it to prestest. However, the problem with using the workspace is that I need the node_modules separated for this specific case. Do you have an idea how to do this? When a ran yarn install with workspace all node_modules goes to the root.

I need it separated to copy it into the docker.

Copy link
Owner

@oblador oblador left a comment

Choose a reason for hiding this comment

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

Super cool! Make sure to add empty lines on all files too to make GitHub happy 👍

@thiagodmont
Copy link
Contributor Author

Hey @oblador, do you have any news on this? Can I help with any other change or clarifying anything in the PR?

Thanks :)

@thiagodmont
Copy link
Contributor Author

Hey @oblador did the requested changes, please let me know if you have any other suggestion.

@oblador
Copy link
Owner

oblador commented Jan 25, 2024

Awesome, thanks!

@oblador oblador merged commit bbeb64a into oblador:master Jan 25, 2024
4 checks passed
@oblador
Copy link
Owner

oblador commented Jan 25, 2024

Released in 0.34.0

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