Skip to content

fix(install): lifecycle script changes - #8943

Merged
Jarred-Sumner merged 40 commits into
mainfrom
dylan/lifecycle-script-changes
Mar 8, 2024
Merged

Jarred-Sumner merged 40 commits into
mainfrom
dylan/lifecycle-script-changes

Conversation

@dylan-conway

@dylan-conway dylan-conway commented Feb 16, 2024 •

Copy link
Copy Markdown
Member

What does this PR do?

  • Breaking change: if trustedDependencies exists in package.json, bun install will no longer pull from the default list and will only run scripts from the trustedDependencies list. If the list is empty, then no scripts will run. The default trusted dependencies list is only used when trustedDependencies does not exist in package.json.

  • --trust. This flags tells bun add to automatically trust the dependency and all it's dependencies, running their lifecycle scripts and adding to trustedDependencies in package.json.

  • warning for skipped lifecycle scripts

Screenshot 2024-03-06 at 11 26 43 PM

Also included in this pr is bun pm trusted. This command without any args will print the current trusted and untrusted dependencies with scripts:
Screenshot 2024-03-07 at 2 25 56 AM

If you provide package names, they will be added to trustedDependencies and their scripts will run. In the example above, running bun pm trust es5-ext will add this to your package.json:

...
trustedDependencies: [
    "es5-ext"
]
...

You can also pass --all to trust all available untrusted dependencies, and --default to list the default trusted dependencies list.

The breaking changes in this pr will be disabled until v1.1.0.

How did you verify your code works?

new and existing tests

@github-actions

github-actions Bot commented Feb 16, 2024 •

Copy link
Copy Markdown
Contributor

❌ @dylan-conway 1 files with test failures on bun-darwin-aarch64:

View test output

#1f33830bb01114b03d1e481f21269a852316922b

@github-actions

github-actions Bot commented Feb 16, 2024 •

Copy link
Copy Markdown
Contributor

❌ @dylan-conway 1 files with test failures on linux-x64:

View test output

#1f33830bb01114b03d1e481f21269a852316922b

@github-actions

github-actions Bot commented Feb 16, 2024 •

Copy link
Copy Markdown
Contributor

❌ @dylan-conway 1 files with test failures on linux-x64-baseline:

View test output

#1f33830bb01114b03d1e481f21269a852316922b

@github-actions

github-actions Bot commented Feb 16, 2024 •

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Feb 16, 2024 •

Copy link
Copy Markdown
Contributor

❌🪟 @dylan-conway, there are 12 test regressions on Windows x86_64

  • test\cli\install\bun-upgrade.test.ts
  • test\cli\install\migration\migrate.test.ts
  • test\cli\run\transpiler-cache.test.ts
  • test\js\bun\dns\resolve-dns.test.ts
  • test\js\bun\http\fetch-file-upload.test.ts
  • test\js\bun\http\bun-server.test.ts
  • test\js\bun\shell\shelloutput.test.ts
  • test\js\bun\shell\throw.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\process\process-args.test.js
  • test\js\web\fetch\body-stream-excess.test.ts
  • test\js\web\fetch\body-stream.test.ts

Full Test Output

@dylan-conway
dylan-conway marked this pull request as ready for review March 7, 2024 11:01
Comment thread src/install/install.zig Outdated
Comment thread src/install/install.zig Outdated
Comment thread src/install/install.zig Outdated
Comment thread src/js_ast.zig Outdated
return array;
}

/// debug.assert the array only contains strings before calling

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make this an actual debug assert for this function. or uh, i see you do a runtime check for that... so is it an assertion or? i'd like osmething better than this comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the debug assert inside this function. Also did the same for alphabetizeProperties. Originally I had the runtime check for extra safety

Comment thread src/install/lockfile.zig Outdated
// 0 - package from old lockfile, needs update
// 1 - does not have install scripts
// 2 - has install scripts
__has_install_script: u8,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of a magic number, can this be an enum(u8). it would also be nice to add this into the comment on line 4709 when we bump the lockfile version, we should reorder this to:

also proper documentation comments are /// but that is less relevant.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to enum


await rm(join(packageDir, "node_modules", "electron", "preinstall.txt"), { force: true });

// lockfile should save evenn though there are no changes to trustedDependencies due to

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// lockfile should save evenn though there are no changes to trustedDependencies due to
// lockfile should save even though there are no changes to trustedDependencies due to

Comment thread src/install/install.zig
else => return,
else => {
if (comptime Environment.allow_assert) {
@panic("bad");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad

Comment thread src/install/install.zig Outdated
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
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.

bun install doesn't support file:. path in package.json dependencies

3 participants