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

chore(pre-req): clean up pre-request script sdk objects - INS-3379 #7172

Merged
merged 23 commits into from
Mar 22, 2024

Conversation

ihexxa
Copy link
Contributor

@ihexxa ihexxa commented Mar 13, 2024

Please review this PR (#7171) firstly as current one is based on it

Changes:

  • enable eval()
  • add more tests
  • enable requiring node.js modules
  • make console.log outputs more friendly
  • enable requiring npm modules -- will do this in a separated PR
  • enable built-in lodash module
  • avoid exposing fs as it is for internal usage

How to test

Run this script and check if it runs well:

// eval the express and set it
const evalResult = eval('8+8');
insomnia.environment.set('evalResult', evalResult);
console.log({
  field1: {field2: 'boom'}
});

packages/insomnia/src/hidden-window.ts Dismissed Show resolved Hide resolved
@ihexxa ihexxa self-assigned this Mar 13, 2024
@ihexxa ihexxa force-pushed the pre-req-cleanup-3 branch 2 times, most recently from 7d37e86 to eb347c1 Compare March 15, 2024 14:26
@ihexxa ihexxa changed the title chore: clean up pre-request script sdk objects chore: clean up pre-request script sdk objects - INS-3379 Mar 15, 2024
@ihexxa ihexxa changed the title chore: clean up pre-request script sdk objects - INS-3379 chore(pre-req): clean up pre-request script sdk objects - INS-3379 Mar 18, 2024
Copy link
Contributor

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

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

For exposing node specific modules:

  • We need to document those alongside the node version that we support. This has the potential to block us from upgrading electron and node because of breaking changes to pre-request scripts

For exposing installed modules from npm:

  • Similar to node but with greater risk of breaking changes. We should add these one by one once we figure out the API documentation story and how to communicate breaking changes. e.g. what happens when a version of a package we expose has a vulnerability or some reason to be updated but it contains breaking changes?

packages/insomnia/src/hidden-window-preload.ts Outdated Show resolved Hide resolved
@gatzjames
Copy link
Contributor

Let's remove the external npm modules from this PR (lodash, moment etc) and merge the changes.
We can introduce those in another PR to have a conversation about external npm modules, bundling them and avoiding installing them in the app (shouldn't be accessible by renderer/main code)

@ihexxa
Copy link
Contributor Author

ihexxa commented Mar 22, 2024

The external modules have been removed and I will set up another PR for external modules and then lets focus on it.

marckong
marckong previously approved these changes Mar 22, 2024
Copy link
Contributor

@marckong marckong left a comment

Choose a reason for hiding this comment

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

Giving a thumbs up for now to ship this feature, but I would say we should explore a path that uses packages installed in the user's operating system instead of shipping these modules in the Insomnia's executable package.

The reason is we cannot support all new modules with different versions that users request. We should probably allow them to select node runtime version and load the packages that are installed, so the Insomnia client is literally a shell that runs these javascript "files" instead. However, I understand this is quite difficult challenge to be tackled

@marckong
Copy link
Contributor

I am also still learning the context of pre-request feature implementation, so please consider this approval as a half.

});
},
},
// {
Copy link
Contributor

Choose a reason for hiding this comment

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

are these commented out for a reason?

@ihexxa ihexxa merged commit a79b01e into develop Mar 22, 2024
7 checks passed
@ihexxa ihexxa deleted the pre-req-cleanup-3 branch March 22, 2024 12:02
@ihexxa
Copy link
Contributor Author

ihexxa commented Mar 22, 2024

The new PR has been create at: #7198

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.

4 participants