fix: serverless failing #162
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andreabadesso
force-pushed
the
fix/serverless-failing
branch
2 times, most recently
from
6da1cd1 to
5ae61b7
Compare
andreabadesso
force-pushed
the
refactor/common-project
branch
from
c19c2c3 to
16ba61f
Compare
andreabadesso
force-pushed
the
fix/serverless-failing
branch
from
5ae61b7 to
fe06242
Compare
andreabadesso
force-pushed
the
refactor/common-project
branch
from
16ba61f to
9d84734
Compare
andreabadesso
force-pushed
the
fix/serverless-failing
branch
from
fe06242 to
9567898
Compare
pedroferreira1
approved these changes
May 2, 2024
| @@ -24,7 +24,9 @@ module.exports = { | |||
| filename: '[name].js', | |||
| }, | |||
| target: 'node', | |||
| externals: [nodeExternals()], | |||
| externals: [nodeExternals({ | |||
| allowlist: [new RegExp("@wallet-service/common*")], | |||
There was a problem hiding this comment.
The bundle gets too big if we allow webpack to bundle all dependencies so we remove them from the bundle (they get loaded in runtime)
But we can't do this with the common module, otherwise it would not be seen by the serverless-monorepo package
tuliomir
reviewed
May 3, 2024
There was a problem hiding this comment.
suggestion: I agree with your suggestion to move on with this PR and investigate the peer dependencies issue through a KTLO.
My recommendation would be to add the issue number to this PR description now, so that it's easier to track these changes in the future.
tuliomir
approved these changes
May 3, 2024
andreabadesso
force-pushed
the
refactor/common-project
branch
from
9d84734 to
25261a1
Compare
andreabadesso
force-pushed
the
fix/serverless-failing
branch
from
9567898 to
4bbbf6b
Compare
pedroferreira1
approved these changes
May 29, 2024
andreabadesso
force-pushed
the
fix/serverless-failing
branch
from
c2fe235 to
8deea8b
Compare
…methods in wallet-service package
andreabadesso
force-pushed
the
fix/serverless-failing
branch
from
8deea8b to
b21d3f3
Compare
andreabadesso
added a commit
that referenced
this pull request
May 30, 2024
* chore: added common package * chore: using wallet-lib from daemon resolution * chore: installed shared dependencies on root project, using peerDependencies * refactor: using addAlert method from common utils * chore: updated package.json with missing deps * refactor: using types from commons on wallet-service * chore: re-added sequelize to root * refactor: removed isNftAutoReviewEnabled from services * tests: mocked assertEnv * tests: mocked assertEnv on integration tests * chore: removed nft utils * refactor: removed old addAlerts from the wallet-service package * chore: wallet-lib is now a peerDependency in wallet-service package * refactor: logger is now a required param in addAlert, refactored all methods in wallet-service package * docs: added missing hathor header * refactor: invalid type for metadata on addAlert * tests: passing mocked logger * chore: updated gitignore to ignore env files * chore: revert eslint package updates * refactor: using types from commons on wallet-service * refactor: removed unused commented line * chore: missing package in daemon * refactor: using transaction type from common * refactor: bitcoinjs is not a shared dep * chore: added a comment explaining that logger is a param temporarily * refactor: removed default from metadata * chore: lint fixes and package ordering * feat: call onNewNftEvent when a new NFT tx is received (#150) * feat: added nft utils * tests: added tests for common utils * chore: added common module to CI * refactor: moved used types to common package * tests: removed nft utils * tests: fixed nft tests on txProcessor * tests: mocking network on getconfig * tests: fixed nft tests on txProcessor * refactor: passing logger to invoke nft handler * refactor: no need to ignore ts * chore: removed jest script, instead using only test * chore: added hathor header * refactor: using common utils on txProcessor * tests: nft utils using old syntax * tests: skipped txProcessor tests * tests: fixed txProcessor tests * refactor: using isAuthority from common utils * refactor: using isAuthority from common types * refactor: using assertEnvVariablesExistance from common project * refactor: getting CREATE_NFT_MAX_RETRIES from env * docs: updated docstrings on nft utils * chore: removed events/nftCreationTx.ts * refactor: invalid import locations * refactor: remove unused lambdas (#155) * tests: fixed nft tests on txProcessor * refactor: removed all methods related to the old wallet-service txProcessor * tests: fixed failing test * fix: serverless failing (#162) * refactor: logger is now a required param in addAlert, refactored all methods in wallet-service package * chore: changed module resolution and several package locks * chore: added common library to externals whitelist and whitelisted it in tsloader * chore: wallet-service common package moved from direct path to workspace * chore: added common as a peer dependency and restored peer dependencies * chore: removed peer dependencies as they were not working with serverless-monorepo * chore: added comment on hoisting limits * chore: added comment explaining externals in webpack
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Serverless was failing to package because of multiple issues related to the new yarn workspaces monorepo structure:
Upon investigation, I found out that this was happening because it yarn workspaces symlinks internal modules to the package's
node_modulesandserverless-monorepowas cleaning itThe fix for this issue was adding it as a peer dependency, which actually copied the dependency to the
node_modulesfolder and also adding it to be parsed by thets-loaderThis is a weird one, but after we solved the issue with the common module, the package was failing in runtime because of errors on the imports
I wasn't to dive deep into why this was the case, but moving the peer dependencies to be dependencies fixed this issue.
My suggestion is to move on with this PR because it is stopping us from fixing a bug affecting our production server (NFT metadata is not being created on S3) and investigate in a KTLO in the near future.
The side effect is only that
yarn installwill take a little longer because it will not hoist the peer dependencies into the root package.Acceptance Criteria
Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged