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: nodejs_npm rewrite local deps #215

Conversation

cBiscuitSurprise
Copy link

@cBiscuitSurprise cBiscuitSurprise commented Dec 10, 2020

Description of changes:
In line with this design para, I'd like to work on fixing this as it's currently making my life very difficult.

Steps to remove WIP::

  • Proof of concept on sample structure
  • Expand sample structure and source to include recursive local modules
  • Extract common steps from NodejsNpmPackAction and DependencyUtils.package_local_dependency into common function
  • Add / Update tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@cBiscuitSurprise cBiscuitSurprise marked this pull request as draft December 10, 2020 05:28
@cBiscuitSurprise cBiscuitSurprise force-pushed the feature/nodejs_npm/step-2-rewrite-local-deps branch from f88bccc to 48ff701 Compare December 10, 2020 22:03
@cBiscuitSurprise cBiscuitSurprise marked this pull request as ready for review December 10, 2020 22:12
@cBiscuitSurprise cBiscuitSurprise changed the title wip: feat: nodejs_npm rewrite local deps feat: nodejs_npm rewrite local deps Dec 10, 2020
@cBiscuitSurprise
Copy link
Author

@elbayaaa @mgrandis How do I go about getting a review of this PR?

@mgrandis
Copy link
Contributor

mgrandis commented Jan 7, 2021

Thanks for you contribution @cBiscuitSurprise!
I am currently reviewing it.

@mgrandis
Copy link
Contributor

mgrandis commented Jan 19, 2021

Thanks again for your contribution @cBiscuitSurprise!
I have added a few remarks, I'm open to discuss them.

Copy link

@hoffa hoffa left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Added a few minor comments, but looking great otherwise.

aws_lambda_builders/workflows/nodejs_npm/utils.py Outdated Show resolved Hide resolved
aws_lambda_builders/workflows/nodejs_npm/utils.py Outdated Show resolved Hide resolved
aws_lambda_builders/workflows/nodejs_npm/utils.py Outdated Show resolved Hide resolved
org_file = "package.json"
bak_file = org_file + ".bak"

for (root, _, files) in self.osutils.walk(self.artifacts_dir):
Copy link

Choose a reason for hiding this comment

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

Why is the walk necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

During the building process we update the package.json files to point to the new packed dependencies.
We make backups of the original files.
At the end of the process we walk the dirs to revert the files to their backup so that the files we upload are the original ones. (#187)

@mgrandis
Copy link
Contributor

Quick follow-up:
Referencing local dependencies doesn't work when building in a container.
We are discussing how to handle this situation.

@mgrandis
Copy link
Contributor

@cBiscuitSurprise
Just a little update to ask you about your personal usage of local dependencies.
Are they usually inside the project directory (next to your template) or can they be anywhere on your hard drive?
I know this PR handles both but this question would help for our design on mounting them inside the build container.
Thanks!

@cBiscuitSurprise
Copy link
Author

@mgrandis At this point, we've only included our dependencies as git-submodules (and thus they live within the project directory). I hadn't thought about the generic case. I could see some potential there for us, but at this point, it's not our requirement.

@moelasmar
Copy link
Contributor

Thanks @cBiscuitSurprise for your contribution, I will mark this PR as draft till we finalize the local dependencies usage when building in containers

@moelasmar moelasmar marked this pull request as draft April 25, 2022 16:37
@jfuss
Copy link
Contributor

jfuss commented Aug 26, 2022

I chatted with the team about this. I know a lot of effort was put into this PR but we are not going to accept this contribution.

The team has another way we want to tackle local dependencies across many of the runtimes we support with build. The solution will allow us for a wider support than just Node. My understanding of the direction we are wanting to take is build within the source directory. This should solve for more problems we see with build than just local dependencies. I do not have an ETA to share with the community at this time.

Thanks for the contribution and time invested in this PR.

@jfuss jfuss closed this Aug 26, 2022
@0cv
Copy link

0cv commented Oct 24, 2022

@jfuss any place (issue, pr, etc.) where we can track the progress on the resolution of this issue?

@fabge
Copy link

fabge commented Dec 7, 2022

It'd be nice to have some transparency on the current state of this issue. We are having trouble here as well. @jfuss

@jarnohenneman
Copy link

Hi @jfuss , is there a workable solution available? or an update maybe

@fabge
Copy link

fabge commented Feb 8, 2023

@jfuss we would all appreciate and update on the current development status

@kirilgorbachov
Copy link

@jfuss Any update on this one?

@AdrKacz
Copy link

AdrKacz commented Feb 24, 2023

@jfuss since you closed the PR to resolve the issue we cannot see any improvement. What is the status how the feature you told about?

@jarnohenneman
Copy link

@jfuss apologies for the second ping on this. Any clarification would be much appreciated;

cc; @moelasmar @mgrandis @hoffa

@alphonse92
Copy link

No updates?

@SeongrokLee
Copy link

I got a solution to resolve it. if you move modules from devDependencies to dependencies, it's works well. But I don't know why doesn't work in case of dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.