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

fix #20675 by adding an esbuild plugin to inject env variables #20834

Merged
merged 5 commits into from
Jan 31, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jan 30, 2023

Issue: #20675

add an esbuild plugin to the builder-manager that injects environment variables

@socket-security
Copy link

socket-security bot commented Jan 30, 2023

Socket Security Pull Request Report

👍 No dependency changes detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Shell access ✅ 0 issues
Uses eval ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
New author ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
AI detected malware ✅ 0 issues

Powered by socket.dev

@ndelangen
Copy link
Member Author

ndelangen commented Jan 30, 2023

esbuild-plugin-environment should be using [email protected] instead of [email protected]

🤔

@webdeveric I'm trying to use your package: https://github.com/webdeveric/esbuild-plugin-environment/blob/master/package.json
But it's giving me the above listed error in CI. Do you have any advice for me?
It seems related to:
https://github.com/webdeveric/esbuild-plugin-environment/blob/master/package.json#L27

@webdeveric
Copy link

@ndelangen The check-package-manager cli just ensures the correct package manager is being used in the project. It should skip running if it detects that it is being called from a dependency. Perhaps there is an edge case I missed.

Can you rerun the failed GH Action with debug logging turned on? check-package-manager recognizes that debug logging is on and will print out debug messages.

@IanVS
Copy link
Member

IanVS commented Jan 30, 2023

Hah, of course it passed as soon as I enabled debug mode... Going to try running them all again without, to see if debug actually makes it pass.

@webdeveric
Copy link

@IanVS I made some fixes to check-package-manager that resolved the issue on Windows. I'm using npx to call the cli so it'll always get the latest version, which is why it is passing now for you.

@@ -54,6 +54,7 @@
"ejs": "^3.1.8",
"esbuild": "^0.16.4",
"esbuild-plugin-alias": "^0.2.1",
"esbuild-plugin-environment": "^0.2.2",

Choose a reason for hiding this comment

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

I published version 0.2.3 of this plugin.
If you update to that version, you will get rid of some transitive dependencies.

@ndelangen
Copy link
Member Author

We should QA that this feature actually does what we intent it should do.

@ndelangen
Copy link
Member Author

@webdeveric I ended up using the define property instead, after local testing, it seemed the plugin wasn't replacing references in the code.

I do very much appreciate the support you've given. ❤️

@webdeveric
Copy link

@ndelangen If you have a reproducible bug, please open an issue and I'll take a look.

But on first glance, it looks like you were calling JSON.stringify when you didn't need to.
My plugin does that for you.

environmentPlugin({
  ...stringifyProcessEnvs(envs),
  NODE_ENV: JSON.stringify(process.env.NODE_ENV),
}),

Should have been (untested)

environmentPlugin(envs),
environmentPlugin(['NODE_ENV']),

@ndelangen
Copy link
Member Author

Ow, I needed to invoke the plugin twice? once with the object and once with the keys?

I just assumed those were 2 separate use-cases.

@webdeveric
Copy link

@ndelangen They are different use cases, one for user provided default values, and the other (array of keys) uses empty string as the default.

I was just removing the JSON.stringify from your code in my example above.

@ndelangen ndelangen merged commit 22fa110 into next Jan 31, 2023
@ndelangen ndelangen deleted the norbert/fix-20675 branch January 31, 2023 14:04
@@ -6,3 +6,6 @@ addons.setConfig({
renderLabel: ({ name, type }) => (type === 'story' ? name : startCase(name)),
},
});

console.log(process.env.STORYBOOK_FOO);
console.log(process.env);
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to leave these console.log statements here, @ndelangen ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not 😨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants