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

fix(lambda-at-edge): for serverless-trace target, exclude files that … #758

Merged
merged 4 commits into from Nov 2, 2020

Conversation

dphang
Copy link
Collaborator

@dphang dphang commented Nov 2, 2020

that are typescript sources.

Assuming that this should fix #461? It works to fix TypeScript projects at least.

I can add some e2e tests for this too later.

@dphang dphang marked this pull request as draft November 2, 2020 08:50
@dphang
Copy link
Collaborator Author

dphang commented Nov 2, 2020

@danielcondemarin not sure this is the right fix but for my project this excludes all ts/tsx files that were trying to be copied with same src and destination (hence failing). However it fails the test, not sure how we can test it?

Should we just exclude .ts/.tsx files instead?

@danielcondemarin
Copy link
Contributor

Doesn't look right to me @dphang . That would be excluding traces that are within the app itself and not in node_modules. I suspect that will break for some apps.

What are the ts / tsx files you're seeing? Dependencies of which page? I wouldn't expect to see a ts / tsx trace after building the app.

@dphang
Copy link
Collaborator Author

dphang commented Nov 2, 2020

@danielcondemarin if you see that issue I had mentioned somehow the serverless trace was trying to copy TSX pages from and to the same directory and it was failing at the file copy command due to source and destination being the same.

It worked for my typescript project but that was a quick hack at the time (a few months ago) - I also didn't understand much about how this component works.

Let me go and look more closely to make a proper fix.

@dphang
Copy link
Collaborator Author

dphang commented Nov 2, 2020

@danielcondemarin I think the @zeit/node-file-trace is somehow returning the app's pages TS/TSX files which shouldn't be copied. Not sure if it's a bug on this package, but I think we can safely exclude these files. Also looking into @vercel/nft since it mentions that @zeit/node-file-trace is deprecated, maybe it's fixed in the new version.

@dphang dphang marked this pull request as ready for review November 2, 2020 22:24
@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #758 into master will decrease coverage by 0.03%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #758      +/-   ##
==========================================
- Coverage   79.69%   79.66%   -0.04%     
==========================================
  Files          55       55              
  Lines        1798     1800       +2     
  Branches      390      391       +1     
==========================================
+ Hits         1433     1434       +1     
- Misses        307      308       +1     
  Partials       58       58              
Impacted Files Coverage Δ
packages/libs/lambda-at-edge/src/build.ts 93.72% <50.00%> (-0.40%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e252c2e...7cefe87. Read the comment docs.

@@ -131,6 +131,13 @@ class Builder {
.filter((file) => {
// exclude "initial" files from lambda artefact. These are just the pages themselves
// which are copied over separately

// For TypeScript apps, somehow nodeFileTrace will generate filelist with TS or TSX files, we need to exclude these files to be copied
// as it ends up copying from same source to destination.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also we could prevent FS from copying if src == dest but I think this should work for now.

@danielcondemarin
Copy link
Contributor

Would you mind upgrading to https://github.com/vercel/nft? Hopefully shouldn't be breaking.

@dphang dphang merged commit 2d72f4f into master Nov 2, 2020
@delete-merged-branch delete-merged-branch bot deleted the dphang/fix-serverless-trace branch November 2, 2020 23:10
@dphang
Copy link
Collaborator Author

dphang commented Nov 2, 2020

Would you mind upgrading to https://github.com/vercel/nft? Hopefully shouldn't be breaking.

Yea I tried it but it seems like it broke many unit tests, need more time to investigate it. It also did not fix the TS/TSX copying issue 😢

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.

Error: Source and destination must not be the same. When using useServerlessTraceTarget
2 participants