-
Notifications
You must be signed in to change notification settings - Fork 34
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
β¬ Error in the date selector test + π WSOD in prod build #1113
Comments
The diffs are fairly significant and at least at first glance, don't seem to affect the
So it is not obvious what to revert. |
However, if I copy over the package-lock.json into a fresh copy of the phone repo, the setup process installs the exact versions and the test works
results in
|
Note further that per the npm documentation, https://docs.npmjs.com/cli/v10/configuring-npm/package-lock-json?v=true
So it sounds like maybe we should check this working version into the source code. I know that we tried to do that before, but then (probably due to an older version of npm), every run of But with this npm behavior, maybe we should re-think that, and check in the |
So we can have the CI use known good versions of the dependencies. Without this change, the underlying libraries can change based on the loosely defined semver in the `package.json`, which can cause unexpected failures. An example of such a failure is: e-mission/e-mission-docs#1113 Copying over the `package-lock.json` overrides `package.json` and forces the build to use the exact versions specified e-mission/e-mission-docs#1113 (comment) Note that `package-lock.json` is apparently designed to checked in to source control. It used to be checked in (thanks to a contribution from the UNSW team) but I removed it because every run of `npm install` would override it, resulting in confusing merge conflicts. However, it looks like `npm` has now changed its behavior so it respects the `package-lock.json` if it exists. e-mission/e-mission-docs#1113 (comment) So we restore it by copying in known good versions - from https://github.com/e-mission/e-mission-phone/actions/runs/12448538357 for the "serve" case and from the last successful internal build of NREL OpenPATH for the `cordovabuild` case. We also change `bin/configure_xml_and_json.js` to copy these files over as well Testing done: - Copying `package-lock.json` into the repo causes `npm install` to work, and the tests to pass - Testing of the setup changes will happen through CI
So we can have the CI use known good versions of the dependencies. Without this change, the underlying libraries can change based on the loosely defined semver in the `package.json`, which can cause unexpected failures. An example of such a failure is: e-mission/e-mission-docs#1113 Copying over the `package-lock.json` overrides `package.json` and forces the build to use the exact versions specified e-mission/e-mission-docs#1113 (comment) Note that `package-lock.json` is apparently designed to checked in to source control. It used to be checked in (thanks to a contribution from the UNSW team) but I removed it because every run of `npm install` would override it, resulting in confusing merge conflicts. However, it looks like `npm` has now changed its behavior so it respects the `package-lock.json` if it exists. e-mission/e-mission-docs#1113 (comment) So we restore it by copying in known good versions - from https://github.com/e-mission/e-mission-phone/actions/runs/12448538357 for the "serve" case and from the last successful internal build of NREL OpenPATH for the `cordovabuild` case. We also change `bin/configure_xml_and_json.js` to copy these files over as well Testing done: - Copying `package-lock.json` into the repo causes `npm install` to work, and the tests to pass - Testing of the setup changes will happen through CI
Note that since I copied over the |
Ah the CI build uses But given that the |
I'm getting the same error even after removing |
Ah this is because of my fix from Dec 21st. That was the day of the last release, so this is the first time I am building with this change. Not sure why |
|
This was originally added in 4566cc0 and seems to work fine for the `serve` case But it doesn't work for the `cordovabuild` case e-mission/e-mission-docs#1113 (comment) e-mission/e-mission-docs#1113 (comment) Adding it, although I still don't know why the `serve` case did not need it
As a next issue, although the dev build works well, the prod build generates a WSOD.
Since it happens in the prod build and not in the dev build, it is likely due to transpiling and is probably due to and our use of RN web instead of RN Since the dev build works, and we added the additional transpiling to get unit tests (that run against the dev build) to work, I will try to fix by enabling that preset only for dev builds |
I can confirm manually removing that preset removes the WSOD on the release build. I am pushing out a new release with this change right now, and will fix in the codebase next. |
Changing
works Now let's make it dependent on the environment. |
However, adding the log
|
Using However, trying to use
which are consistent with the I guess it may be because when testing, the environment is not
|
Remaining cleanups for this task are:
|
In the previous commit, 1089679 we copied over the cordovabuild package-lock from the internal build of NREL OpenPATH. However, when trying to get the internal build to complete, for this release, I moved out the existing package-lock and regenerated, as an attempt to fix e-mission/e-mission-docs#1113 (comment) After adding the new module (in b9e9a37) all of the dependency versions also changed. Since `package-lock.json` is supposed to represent the checked in "time travel" version of the modules actually used, we update this to reflect the current module versions used as well.
In #1193, I added `module:@react-native/babel-preset` That caused the unit tests to work, but caused a WSOD with the release build e-mission/e-mission-docs#1113 (comment) So we exclude the preset for production builds I tried to add it only to dev builds, but the `process.env.ENV` is not set correctly so I had to switch the condition e-mission/e-mission-docs#1113 (comment) Testing done: ``` $ npm run test Test Suites: 27 passed, 27 total Tests: 182 passed, 182 total Snapshots: 0 total Time: 10.537 s Ran all test suites. ```
In #1193, I added `module:@react-native/babel-preset` That caused the unit tests to work, but caused a WSOD with the release build e-mission/e-mission-docs#1113 (comment) So we exclude the preset for production builds I tried to add it only to dev builds, but the `process.env.ENV` is not set correctly so I had to switch the condition e-mission/e-mission-docs#1113 (comment) Testing done: ``` $ npm run test Test Suites: 27 passed, 27 total Tests: 182 passed, 182 total Snapshots: 0 total Time: 10.537 s Ran all test suites. ```
The jest tests for the date selector have been failing for the past week. None of the subsequent changes are related to date selection, so I suspect this is due to a change in the version.
The text was updated successfully, but these errors were encountered: