-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Handle process.env correctly in build #2267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only looked at the mobx.module.js
bundle, it definitely solves #2266
The package has the process.env
which lets the bundler do the rest (replace the process.env.NODE_ENV
with the correct environment)
@@ -29,6 +29,7 @@ | |||
"_prepublish": "yarn small-build", | |||
"quick-build": "tsc --pretty", | |||
"small-build": "node scripts/build.js", | |||
"build": "node scripts/build.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as "small-build"
I would remove one of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to standardize to build
but not to remove small-build
in case someone still uses that for something.
@@ -121,6 +114,19 @@ function writePackage(versionPath, version) { | |||
) | |||
} | |||
|
|||
function writeIndex(versionPath) { | |||
fs.writeFileSync( | |||
path.resolve(versionPath, "lib", "index.js"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not critical, but IMHO it's better to write this file somewhere in the codebase and just copy it as part of the build process. It will make sure that we're not making any mistake and makes the code a bit clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, was thinking about it but did not want to come up with some artificial filename or folder where to keep it. It's such a small thing it's ok.
Can confirm that it also solves #2264 |
Thanks, guys. I will merge and release tomorrow, stick to X.15.2 till then, please. |
Fixes #2266 #2264
Here is the build output it creates, looks ok to me, but I could use a second pair of eyes.
v5.zip
cc @mweststrate @ranyitz @le0nik