-
Notifications
You must be signed in to change notification settings - Fork 819
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(amplify-provider-awscloudformation): custom transformer imports #3236
fix(amplify-provider-awscloudformation): custom transformer imports #3236
Conversation
Codecov Report
@@ Coverage Diff @@
## master aws-amplify/amplify-cli#3236 +/- ##
=======================================
Coverage 59.39% 59.39%
=======================================
Files 254 254
Lines 12590 12590
Branches 2637 2637
=======================================
Hits 7478 7478
Misses 4780 4780
Partials 332 332 Continue to review full report at Codecov.
|
a14e403
to
445587b
Compare
const projectRootPath = context.amplify.pathManager.searchProjectRootPath(); | ||
const { createRequireFromPath } = require('module'); | ||
const projectRequire = createRequireFromPath(projectRootPath); | ||
|
||
if (tempModulePath.startsWith('./')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that there is no concept like global require
and local require
.
module.paths
is calculated by caller's path and NODE_PATH
.
https://stackoverflow.com/questions/15636367/nodejs-require-a-global-module-package
I think this issue is important one. Currently there is no reasonable way to use custom transformers coming from npm package. I realized that after publishing a package. |
445587b
to
c8e93ff
Compare
c8e93ff
to
177f12a
Compare
@hirochachacha could you please elaborate on what is NOT working with the current implementation? |
@attilah I tried to use my custom transformer.
Then I saw
The following code is a short summary of current implementation.
You can check that this line of code cannot load And next one is proposed solution.
|
Recent node supports directory path through trailing slash, but stable node 10.x doesn't include this change. So we need to handle it manually. |
@attilah is it still unclear? |
Any update on this? |
@attilah it seems that you attempted to fix: aws-amplify/docs#992 (comment), as per: aws-amplify/docs#992 (comment). However, it doesn't seem to work for locally installed packages. For clarity here's what happens on transform.conf.json {
"Version": 5,
"ElasticsearchWarning": true,
"transformers": ["graphql-auto-transformer"]
}
There is a workaround here: hirochachacha/graphql-auto-transformer#2 However, that adds additional maintenance complexity and comes with its own set of issues. |
@attilah I still don't understand why you want to ignore this issue. Maybe you don't think this is an issue because you've already tested custom imports feature? https://github.com/amazon-archives/aws-reinvent-2019-mobile-workshops/tree/master/MOB405
You expect that "reinvent-string-validator" module will be loaded from But in fact, it will be loaded from "<project_dir>/../reinvent-string-validator" |
@hirochachacha would there be a way to provide the full path in the config? For example: "transformers": [
"../node_modules/graphql-auto-transformer"
] My various attempts have been unsuccessful. |
@andreialecu you mean, except using NODE_PATH?
It emulates directory structure of https://github.com/amazon-archives/aws-reinvent-2019-mobile-workshops/tree/master/MOB405 |
Using absolute path should work also. |
I got it, the following works without linking:
It's not ideal having to hardcode the root folder name in it though. |
Yep, I hope these work-arounds prove that this is, obviously, an issue. |
@hirochachacha thanks for the PR and for finding this issue. I tested the proposed solution with your published package and it failed with globally installed packages (when using NVM) and if the package was referenced with The I did some refactoring by leveraging @hirochachacha, @andreialecu, @sbue, test my commit if it works for your use cases and to make sure this time we get it right. @hirochachacha as previously we asked, please open an issue before opening a PR to be able to discuss the given issue and prioritize it better. |
Current implementation misuses createRequireFromPath API. Unfortunately this API doesn't support directory path well. For example, ``` createRequireFromPath("/my/project")("./node_modules/mylib") ``` isn't same as ``` require("/my/project/node_modules/mylib") ``` Actually, it's ``` require("/my/node_modules/mylib") ```
That's probably working as intended.
As you can see, it includes both local and global paths. So, I'd like to change dot import as usual one to support project specific transformer like: aws-amplify/amplify-category-api#447 Hope it's clear now. |
3de4c1b
to
2053463
Compare
@hirochachacha it is environment dependent, by default you don't get the nvm one, in my setup I don't have it, here is the [
"<root>/amplify-cli/packages/amplify-cli/bin/node_modules",
"<root>/amplify-cli/packages/amplify-cli/node_modules",
"<root>/amplify-cli/packages/node_modules",
"<root>/amplify-cli/node_modules",
"<root>/node_modules",
"<home>/node_modules",
"/Users/node_modules",
"/node_modules",
] That's the reason we need the additional global import. |
@attilah I see. That's interesting. I thought you're talking about special handling for './' in the previous code without reading the PR. Sorry about that. Now it looks like it's good although I didn't test it yet. Thank you for the fix. |
@hirochachacha if you encounter a problem just open an issue. |
@yuth please review my changes in this PR. |
But I just wonder why do you need third party libraries? Even though hirochachacha@e1621a8 failed to load global libraries, pulling back hirochachacha@e1621a8#diff-13054d37b1e2418fc273d5352c4bcd76L105-L108 wasn't good enough? |
@attilah @yuth Wait, this's still wrong. https://github.com/sindresorhus/import-from doesn't follow node's module paths, it only resolve specific path. Besides, it doesn't distinguish |
@hirochachacha this change supports:
What should ./ accomplish in addition to this? if you specify ./ that was meant for current project's node_modules. What am I missing? Longer term, transformers will be consumable from plugin packages as that is how you can add extensions to the CLI, but that will require changes, but will work similarly how functions category is doing it now. |
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Current implementation misuses createRequireFromPath API.
Unfortunately this API doesn't support directory path well.
For example,
isn't same as
Actually, it's
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.