fix(amplify-provider-awscloudformation): custom transformer imports #3236

hirochachacha · 2020-01-22T10:01:13Z

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")

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

codecov-io · 2020-01-22T10:09:50Z

Codecov Report

Merging #3236 into master will not change coverage.
The diff coverage is n/a.

@@           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.

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

hirochachacha · 2020-01-22T10:40:33Z

packages/amplify-provider-awscloudformation/lib/transform-graphql-schema.js

            const projectRootPath = context.amplify.pathManager.searchProjectRootPath();
            const { createRequireFromPath } = require('module');
-            const projectRequire = createRequireFromPath(projectRootPath);
-
-            if (tempModulePath.startsWith('./')) {


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

hirochachacha · 2020-01-22T10:45:43Z

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.

attilah · 2020-01-22T18:59:38Z

@hirochachacha could you please elaborate on what is NOT working with the current implementation?

hirochachacha · 2020-01-22T23:06:32Z

@attilah I tried to use my custom transformer.

yarn add -D graphql-auto-transformer
edit amplify/backend/api/<YOUR_API>/transform.conf

    "transformers": [
      "./graphql-auto-transformer"
    ]

amplify-dev api gql-compile

Then I saw

Error: Cannot find module 'graphql-auto-transformer'

The following code is a short summary of current implementation.

require('module').createRequireFromPath(__dirname)("./graphql-auto-transformer")

You can check that this line of code cannot load graphql-auto-transformer from node_modules.

And next one is proposed solution.

require('module').createRequireFromPath(`${__dirname}/noop.js`)('graphql-auto-transformer');

hirochachacha · 2020-01-22T23:23:31Z

https://github.com/nodejs/node/blob/dcba12895ad58275ba5b027c2c5110461dfebf66/lib/internal/modules/cjs/loader.js#L1263-L1268

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.

hirochachacha · 2020-02-19T07:44:51Z

@attilah is it still unclear?

sbue · 2020-03-22T00:45:58Z

Any update on this?

andreialecu · 2020-04-25T10:22:40Z

@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 amplify-cli 4.18.0:

transform.conf.json

{
  "Version": 5,
  "ElasticsearchWarning": true,
  "transformers": ["graphql-auto-transformer"]
}

amplify push output:

There is a workaround here: hirochachacha/graphql-auto-transformer#2

However, that adds additional maintenance complexity and comes with its own set of issues.

hirochachacha · 2020-04-29T09:37:30Z

@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
If so, that's pretty wrong.

https://github.com/amazon-archives/aws-reinvent-2019-mobile-workshops/blob/master/MOB405/Starting%20from%20scratch.md#add-validator-transformer-to-list-of-custom-transformers-in-amplifybackendapivalidatortesttransformconfjson

"transformers": [
    "./reinvent-string-validator"
]

You expect that "reinvent-string-validator" module will be loaded from
"<project_dir>/node_modules/reinvent-string-validator" right?

But in fact, it will be loaded from "<project_dir>/../reinvent-string-validator"
That's the reason you didn't see this issue from your tests. The directory structure concealed it.

andreialecu · 2020-04-29T09:40:58Z

@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.

hirochachacha · 2020-04-29T09:54:01Z

@andreialecu you mean, except using NODE_PATH?

cd <your_project>/..
ln -s <your_project>/node_modules/graphql-auto-transformer

It emulates directory structure of https://github.com/amazon-archives/aws-reinvent-2019-mobile-workshops/tree/master/MOB405
So it should work.

hirochachacha · 2020-04-29T09:59:08Z

Using absolute path should work also.

andreialecu · 2020-04-29T10:00:36Z

I got it, the following works without linking:

    "transformers": [
        "./projectname/node_modules/graphql-auto-transformer"
    ]

It's not ideal having to hardcode the root folder name in it though.

hirochachacha · 2020-04-29T10:06:55Z

Yep, I hope these work-arounds prove that this is, obviously, an issue.

attilah · 2020-05-04T02:33:39Z

@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 ./ prefix in transform.conf.json.

The resolve-from package also uses the same "trick" for noop.js what you applied, I was not aware of that before.

I did some refactoring by leveraging import-global and import-from packages and it worked for me in all the test cases.

@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") ```

hirochachacha · 2020-05-04T03:09:55Z

@attilah

@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 ./ prefix in transform.conf.json.

That's probably working as intended.
Look, we don't have to distinguish local import and global import. local import covers global one.
You can confirm this by printing module.paths.

[ '/Users/hiroshiioka/work/myproject/node_modules',
  '/Users/hiroshiioka/work/node_modules',
  '/Users/hiroshiioka/node_modules',
  '/Users/node_modules',
  '/node_modules',
  '/Users/hiroshiioka/.node_modules',
  '/Users/hiroshiioka/.node_libraries',
  '/Users/hiroshiioka/.nvm/versions/node/v10.18.1/lib/node' ]

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.

attilah · 2020-05-04T03:22:41Z

@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 require.main.paths when the gql-compile running:

[
  "<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.

hirochachacha · 2020-05-04T03:32:54Z

@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.

attilah · 2020-05-04T03:34:55Z

@hirochachacha if you encounter a problem just open an issue.

attilah · 2020-05-04T03:35:40Z

@yuth please review my changes in this PR.

hirochachacha · 2020-05-04T03:48:37Z

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?

hirochachacha · 2020-05-04T05:29:07Z

@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 graphql-auto-transformer and ./graphql-auto-transformer, so there're no chance to accomplish aws-amplify/amplify-category-api#447
Again, I'd suggest to stick with createRequireFromPath or its successor createRequire for providing maximum functionalities to users.

attilah · 2020-05-06T17:56:30Z

@hirochachacha this change supports:

import module/js file by absolute path
import package from current project's node_modules (import-from), package name can be specified with './' prefix or without it
import package from global packages

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.

github-actions · 2021-05-24T21:02:22Z

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 *-help channels for those types of questions.

hirochachacha force-pushed the fix/custom_transformer_imports branch from a14e403 to 445587b Compare January 22, 2020 10:32

hirochachacha commented Jan 22, 2020

View reviewed changes

hirochachacha requested a review from attilah January 22, 2020 10:41

hirochachacha force-pushed the fix/custom_transformer_imports branch from 445587b to c8e93ff Compare January 22, 2020 10:50

hirochachacha mentioned this pull request Jan 22, 2020

using createdAt as a sortKey in @key directive results mandatory id and createdAt fields in CreateItemInput and CreateItem mutation in GraphQL Transform #1657

Closed

hirochachacha force-pushed the fix/custom_transformer_imports branch from c8e93ff to 177f12a Compare January 22, 2020 12:37

attilah self-assigned this Jan 22, 2020

fix: transformer loading to use existing packages and use global prefix

2053463

attilah force-pushed the fix/custom_transformer_imports branch from 3de4c1b to 2053463 Compare May 4, 2020 03:15

attilah approved these changes May 4, 2020

View reviewed changes

attilah assigned yuth May 4, 2020

Attila Hajdrik and others added 3 commits May 6, 2020 11:02

Merge branch 'master' into fix/custom_transformer_imports

68d9873

fix: update yarn.lock

2149a09

fix: test failures for subscription after yarn.lock update

5db5e97

attilah merged commit 7794d73 into aws-amplify:master May 6, 2020

github-actions bot locked as resolved and limited conversation to collaborators May 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(amplify-provider-awscloudformation): custom transformer imports #3236

fix(amplify-provider-awscloudformation): custom transformer imports #3236

hirochachacha commented Jan 22, 2020 •

edited

codecov-io commented Jan 22, 2020 •

edited

hirochachacha Jan 22, 2020

hirochachacha commented Jan 22, 2020 •

edited

attilah commented Jan 22, 2020

hirochachacha commented Jan 22, 2020

hirochachacha commented Jan 22, 2020 •

edited

hirochachacha commented Feb 19, 2020

sbue commented Mar 22, 2020

andreialecu commented Apr 25, 2020

hirochachacha commented Apr 29, 2020

andreialecu commented Apr 29, 2020

hirochachacha commented Apr 29, 2020

hirochachacha commented Apr 29, 2020

andreialecu commented Apr 29, 2020

hirochachacha commented Apr 29, 2020

attilah commented May 4, 2020

hirochachacha commented May 4, 2020

attilah commented May 4, 2020

hirochachacha commented May 4, 2020

attilah commented May 4, 2020

attilah commented May 4, 2020

hirochachacha commented May 4, 2020

hirochachacha commented May 4, 2020

attilah commented May 6, 2020 •

edited

github-actions bot commented May 24, 2021

fix(amplify-provider-awscloudformation): custom transformer imports #3236

fix(amplify-provider-awscloudformation): custom transformer imports #3236

Conversation

hirochachacha commented Jan 22, 2020 • edited

codecov-io commented Jan 22, 2020 • edited

Codecov Report

hirochachacha Jan 22, 2020

Choose a reason for hiding this comment

hirochachacha commented Jan 22, 2020 • edited

attilah commented Jan 22, 2020

hirochachacha commented Jan 22, 2020

hirochachacha commented Jan 22, 2020 • edited

hirochachacha commented Feb 19, 2020

sbue commented Mar 22, 2020

andreialecu commented Apr 25, 2020

hirochachacha commented Apr 29, 2020

andreialecu commented Apr 29, 2020

hirochachacha commented Apr 29, 2020

hirochachacha commented Apr 29, 2020

andreialecu commented Apr 29, 2020

hirochachacha commented Apr 29, 2020

attilah commented May 4, 2020

hirochachacha commented May 4, 2020

attilah commented May 4, 2020

hirochachacha commented May 4, 2020

attilah commented May 4, 2020

attilah commented May 4, 2020

hirochachacha commented May 4, 2020

hirochachacha commented May 4, 2020

attilah commented May 6, 2020 • edited

github-actions bot commented May 24, 2021

hirochachacha commented Jan 22, 2020 •

edited

codecov-io commented Jan 22, 2020 •

edited

hirochachacha commented Jan 22, 2020 •

edited

hirochachacha commented Jan 22, 2020 •

edited

attilah commented May 6, 2020 •

edited