Skip to content

Commit

Permalink
Skip syntax error checking users functions using ESM
Browse files Browse the repository at this point in the history
This is a temporary workaround for a regression in Node.js 16 that causes `node --check` to return false positives when a project is using ESM.

More information here:
GoogleCloudPlatform/functions-framework-nodejs#407

PiperOrigin-RevId: 423439009
Change-Id: I26ad943e8fb55e868ece996bd5d2f7470afbec5a
  • Loading branch information
matthewrobertson authored and Copybara-Service committed Jan 22, 2022
1 parent 9b81ec3 commit 4b22189
Show file tree
Hide file tree
Showing 7 changed files with 130 additions and 2 deletions.
6 changes: 6 additions & 0 deletions builders/gcf/nodejs14/acceptance/acceptance_test.go
Expand Up @@ -141,6 +141,12 @@ func TestAcceptance(t *testing.T) {
MustUse: []string{npm},
MustNotUse: []string{yarn},
},
{
Name: "ESM function",
App: "using_esm",
MustUse: []string{npm},
MustNotUse: []string{yarn},
},
}
for _, tc := range testCases {
tc := tc
Expand Down
6 changes: 6 additions & 0 deletions builders/gcf/nodejs16/acceptance/acceptance_test.go
Expand Up @@ -141,6 +141,12 @@ func TestAcceptance(t *testing.T) {
MustUse: []string{npm},
MustNotUse: []string{yarn},
},
{
Name: "ESM function",
App: "using_esm",
MustUse: []string{npm},
MustNotUse: []string{yarn},
},
}
for _, tc := range testCases {
tc := tc
Expand Down
26 changes: 26 additions & 0 deletions builders/testdata/nodejs/functions/using_esm/function.js
@@ -0,0 +1,26 @@
/**
* Copyright 2020 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Always responds 'PASS'. The use of `export` validates the runtime's support
* for ESM.
*
* @param {!Object} req request context.
* @param {!Object} res response context.
*/
export const testFunction = (req, res) => {
res.send('PASS');
};
3 changes: 3 additions & 0 deletions builders/testdata/nodejs/functions/using_esm/package.json
@@ -0,0 +1,3 @@
{
"type": "module"
}
10 changes: 8 additions & 2 deletions cmd/nodejs/functions_framework/main.go
Expand Up @@ -83,8 +83,14 @@ func buildFn(ctx *gcp.Context) error {
return gcp.UserErrorf("%s does not exist", fnFile)
}

// Syntax check the function code without executing to prevent run-time errors.
ctx.Exec([]string{"node", "--check", fnFile}, gcp.WithUserAttribution)
// TODO(mattrobertson) remove this check once Nodejs has backported the fix to v16. More info here:
// https://github.com/GoogleCloudPlatform/functions-framework-nodejs/issues/407
if skip, err := nodejs.SkipSyntaxCheck(ctx, fnFile); err != nil {
return err
} else if !skip {
// Syntax check the function code without executing to prevent run-time errors.
ctx.Exec([]string{"node", "--check", fnFile}, gcp.WithUserAttribution)
}

l := ctx.Layer(layerName, gcp.BuildLayer, gcp.CacheLayer, gcp.LaunchLayer)
// We use the absolute path to the functions-framework executable in order to
Expand Down
22 changes: 22 additions & 0 deletions pkg/nodejs/nodejs.go
Expand Up @@ -21,6 +21,7 @@ import (
"io/ioutil"
"os"
"path/filepath"
"strings"

"github.com/GoogleCloudPlatform/buildpacks/pkg/cache"
gcp "github.com/GoogleCloudPlatform/buildpacks/pkg/gcpbuildpack"
Expand Down Expand Up @@ -54,6 +55,7 @@ type packageScriptsJSON struct {
// PackageJSON represents the contents of a package.json file.
type PackageJSON struct {
Main string `json:"main"`
Type string `json:"type"`
Version string `json:"version"`
Engines packageEnginesJSON `json:"engines"`
Scripts packageScriptsJSON `json:"scripts"`
Expand Down Expand Up @@ -130,3 +132,23 @@ func CheckCache(ctx *gcp.Context, l *libcnb.Layer, opts ...cache.Option) (bool,

return false, nil
}

// SkipSyntaxCheck returns true if we should skip checking the user's function file for syntax errors
// if it is impacted by https://github.com/GoogleCloudPlatform/functions-framework-nodejs/issues/407.
func SkipSyntaxCheck(ctx *gcp.Context, file string) (bool, error) {
version, err := semver.ParseTolerant(nodeVersion(ctx))
if err != nil {
return false, gcp.InternalErrorf("failed to detect valid Node.js version %s: %v", version, err)
}
if version.Major != 16 {
return false, nil
}
if strings.HasSuffix(file, ".mjs") {
return true, nil
}
if !ctx.FileExists(filepath.Join(ctx.ApplicationRoot(), "package.json")) {
return false, nil
}
pjs, err := ReadPackageJSON(ctx.ApplicationRoot())
return (pjs != nil && pjs.Type == "module"), err
}
59 changes: 59 additions & 0 deletions pkg/nodejs/nodejs_test.go
Expand Up @@ -15,9 +15,11 @@
package nodejs

import (
"path/filepath"
"reflect"
"testing"

gcp "github.com/GoogleCloudPlatform/buildpacks/pkg/gcpbuildpack"
"github.com/GoogleCloudPlatform/buildpacks/pkg/testdata"
)

Expand Down Expand Up @@ -47,3 +49,60 @@ func TestReadPackageJSON(t *testing.T) {
t.Errorf("ReadPackageJSON\ngot %#v\nwant %#v", *got, want)
}
}

func TestSkipSyntaxCheck(t *testing.T) {
testCases := []struct {
name string
version string
packageJSON string
filePath string
want bool
}{
{
name: "Node.js 14",
version: "v14.1.1",
packageJSON: `{"type": "module"}`,
filePath: "index.mjs",
want: false,
},
{
name: "Node.js 16 with mjs",
version: "v16.1.1",
filePath: "index.mjs",
want: true,
},
{
name: "Node.js 16 with modules",
version: "v16.1.1",
packageJSON: `{"type": "module"}`,
want: true,
},
{
name: "Node.js 16 without ESM",
version: "v16.1.1",
want: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
defer func(fn func(*gcp.Context) string) { nodeVersion = fn }(nodeVersion)
nodeVersion = func(*gcp.Context) string { return tc.version }

home := t.TempDir()
ctx := gcp.NewContext(gcp.WithApplicationRoot(home))

if tc.packageJSON != "" {
ctx.WriteFile(filepath.Join(home, "package.json"), []byte(tc.packageJSON), 0744)
}

got, err := SkipSyntaxCheck(ctx, tc.filePath)
if err != nil {
t.Fatalf("Node.js %v: SkipSyntaxCheck(ctx, %q) got error: %v", tc.version, tc.filePath, err)
}
if got != tc.want {
t.Errorf("Node.js %v: SkipSyntaxCheck(ctx, %q) = %t, want %t", tc.version, tc.filePath, got, tc.want)
}
})
}
}

0 comments on commit 4b22189

Please sign in to comment.