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

[Frontend] upgrade node to 12 LTS #2386

Closed
2 tasks done
Bobgy opened this issue Oct 15, 2019 · 8 comments · Fixed by #2830
Closed
2 tasks done

[Frontend] upgrade node to 12 LTS #2386

Bobgy opened this issue Oct 15, 2019 · 8 comments · Fixed by #2830
Assignees
Labels
area/frontend kind/misc types beside feature and bug priority/p1 status/triaged Whether the issue has been explicitly triaged

Comments

@Bobgy
Copy link
Contributor

Bobgy commented Oct 15, 2019

We are currently using node 9 in dockerfile, but 11 in travis CI. We should sync both to the same version.
Latest LTS node 12 should be the best option.

problem we met: #2305 (comment)

However, I don't think this is very urgent.

Our node server runtime is using node 9. We don't have unit tests for node server.
Client UI code run in the browser, so it doesn't matter. Client UI unit tests run in node 11, it's okay.

Having a single version of node will help clear some confusion, but doesn't solve any real problem. I think it's better to first write some basic unit test coverage for the node server before upgrading node.

Places with node version:

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 15, 2019

/area front-end
[tech debt]

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 15, 2019

/priority p2

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 16, 2019

There's an error building frontend docker image related to grpc and node 9, but it goes away with node 12. I don't know why it builds normally during test and release. I think for this reason we should prioritize more on this.

/priority p1

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 16, 2019

/remove-priority p2

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 16, 2019

/priority p1

@eterna2
Copy link
Contributor

eterna2 commented Oct 21, 2019

There's an error building frontend docker image related to grpc and node 9, but it goes away with node 12. I don't know why it builds normally during test and release. I think for this reason we should prioritize more on this.

Because the package-lock.json is not in sync with package.json.

None of the grpc packages are listed inside package-lock.json.

We should target to use npm ci for the Travis test as well as build steps to ensure the dependencies are frozen.

Cuz I noticed there are alot of implicit version dependencies that can fails anytime - i.e. if we use the versions specified in package-lock.json, the build will fail because there are quite a few bugs which are fixed in later versions of the dependencies. npm install default behavior is to install the latest semantic version (which fixed the bugs) rather than the ones specified in package-lock.json - hence we unknowingly fixed the bugs (implicit knowledge - aka. works but we don't know why)

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 23, 2019

@eterna2 thanks for the suggestions, I agree with we should use npm ci instead.
Created issue #2452 to track this problem.

None of the grpc packages are listed inside package-lock.json.

For this specific issue, I don't think that's the root cause. They are in https://github.com/kubeflow/pipelines/blob/master/frontend/src/generated/src/apis/metadata/package-lock.json#L203.
I'm not fully clear why it was put there. I guess the original intention was making that folder a separate package.

@rmgogogo rmgogogo added kind/misc types beside feature and bug status/triaged Whether the issue has been explicitly triaged labels Nov 18, 2019
@Bobgy
Copy link
Contributor Author

Bobgy commented Nov 26, 2019

FYI, I found out the error when building docker image is caused by having node_modules in the working dir. If you delete all of them (like in CI environment), the build will pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend kind/misc types beside feature and bug priority/p1 status/triaged Whether the issue has been explicitly triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants