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

Remove buildOptions access before initialization #341

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Edweis
Copy link

@Edweis Edweis commented Jul 19, 2022

Issue:

With the following settings in serverless.yml:

custom:
  esbuild:
    packager: pnpm
    target: es2019
    minify: true
    bundle: true

When running serverless offline, the offline server starts but when a file is changed, serverless-esbuild runs into an infinite loop and keeps compiling until the process is killed.

image

Investigation

After some investigation, I found that chikidar is initialized with

{
    "options": {
        "ignored": [
            null, // (1) 
            "dist",
            "node_modules",
            null, // (2)
            "node_modules/serverless-esbuild"
        ],
        "awaitWriteFinish": true,
        "ignoreInitial": true
    },
    "defaultPatterns": [
        "./**/*.(js|ts)"
    ]
}

Reading the code, (1)and (2) should actually be respectively .esbuild and .build since they are the default value from serverless-esbuild (link).

The issue is that getCachedOptions ran for the first time before this.outputWorkFolder and this.outputBuildFolder are defined. Hence the cache is populated with null instead.

Fix

This fix, makes this.outputWorkFolder and this.outputBuildFolder initialized with the context value instead of the one from getCachedOptions.

@Edweis Edweis changed the title Remove buildOptions access before initialize Remove buildOptions access before initialization Jul 19, 2022
@samchungy
Copy link
Collaborator

Should be resolved by #337

@dave-irvine
Copy link

I'm having the same issue, but with 1.32.5 which should include #337

At the time getBuildOptions() runs, this.outputWorkFolder and this.outputBuildFolder are undefined, so this line https://github.com/floydspace/serverless-esbuild/blob/master/src/index.ts#L245 puts undefined into the watch.ignore array.

Then these two lines https://github.com/floydspace/serverless-esbuild/blob/master/src/index.ts#L137-L138 are looking for this.buildOptions.outputWorkFolder and this.buildOptions.outputBuildFolder, but those properties don't exist on this.buildOptions.

I'm not sure if this.outputWorkFolder and this.outputBuildFolder are supposed to come from somewhere outside this plugin (are they initialised by Serverless itself?), but certainly in my case they aren't defined.

Possibly this line https://github.com/floydspace/serverless-esbuild/blob/master/src/index.ts#L245 should look more like

ignore: [this.outputWorkFolder || WORK_FOLDER, 'dist', 'node_modules', this.outputBuildFolder || BUILD_FOLDER],

to make use of the constants?

@samchungy
Copy link
Collaborator

I'm having the same issue, but with 1.32.5 which should include #337

At the time getBuildOptions() runs, this.outputWorkFolder and this.outputBuildFolder are undefined, so this line https://github.com/floydspace/serverless-esbuild/blob/master/src/index.ts#L245 puts undefined into the watch.ignore array.

Then these two lines https://github.com/floydspace/serverless-esbuild/blob/master/src/index.ts#L137-L138 are looking for this.buildOptions.outputWorkFolder and this.buildOptions.outputBuildFolder, but those properties don't exist on this.buildOptions.

I'm not sure if this.outputWorkFolder and this.outputBuildFolder are supposed to come from somewhere outside this plugin (are they initialised by Serverless itself?), but certainly in my case they aren't defined.

Possibly this line https://github.com/floydspace/serverless-esbuild/blob/master/src/index.ts#L245 should look more like

ignore: [this.outputWorkFolder || WORK_FOLDER, 'dist', 'node_modules', this.outputBuildFolder || BUILD_FOLDER],

to make use of the constants?

ah yep you are right. Pre-#337 they were initialised in the constructor. Let me think about this and I'll put something up to fix it

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.

3 participants