FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

chore(dev): migrate away from parcel by jsjoeio · Pull Request #3578 · coder/code-server · GitHub

/ code-server Public

chore(dev): migrate away from parcel - #3578

Closed
jsjoeio wants to merge 5 commits into
mainfrom
jsjoeio/upgrade-parcel
ConversationCommits (5)ChecksFiles changed
Closed

chore(dev): migrate away from parcel#3578
jsjoeio wants to merge 5 commits into
mainfrom
jsjoeio/upgrade-parcel

Conversation

jsjoeio commented Jun 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR migrates us away from Parcel by using tsc and browserify to accomplish the same thing.

Originally, we needed to upgrade Parcel from v1 to v2 due to maintainers deprecating v1 and a bunch of security vulnerabilities that we couldn't patch in v1. We were going to use v2 but it has breaking changes. So we decided to replace it with browserify which is a bit more lightweight and does the same thing.

Changes

  • remove parcel
  • use tsc to compile .ts files and browserify to bundle a few files used in the browser
  • reference css files directly in HTML instead of including with js bundles
  • remove minification of browser files (issues with es2015+ support using browserify plugin tinyify)
  • add codecov.yml which allows us to override some default settings

Screenshot

Screenshot after building and running locally

Checklist

  • updated CHANGELOG.md

Related

jsjoeio added this to the 3.11.0 milestone Jun 8, 2021
jsjoeio added the chore Related to maintenance or clean up label Jun 8, 2021
jsjoeio self-assigned this Jun 8, 2021
jsjoeio changed the title title here fix(dev): upgrade to parcel v2 Jun 8, 2021

codecov Bot commented Jun 8, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #3578 (684fe6c) into main (fbba8e8) will decrease coverage by 1.47%.
The diff coverage is 33.33%.

❗ Current head 684fe6c differs from pull request most recent head 5d4e08e. Consider uploading reports for the commit 5d4e08e to get more accurate results

@@            Coverage Diff             @@
##             main    #3578      +/-   ##
==========================================
- Coverage   60.67%   59.19%   -1.48%     
==========================================
  Files          35       35              
  Lines        1790     1784       -6     
  Branches      404      361      -43     
==========================================
- Hits         1086     1056      -30     
- Misses        562      612      +50     
+ Partials      142      116      -26
Impacted Files Coverage Δ
src/browser/pages/vscode.ts 0.00% <0.00%> (ø)
src/browser/register.ts 100.00% <ø> (ø)
src/browser/pages/login.ts 100.00% <100.00%> (+100.00%) ⬆️
src/node/wrapper.ts 22.07% <0.00%> (-11.05%) ⬇️
src/node/vscode.ts 19.48% <0.00%> (-8.02%) ⬇️
src/node/plugin.ts 64.60% <0.00%> (-3.59%) ⬇️
src/node/routes/login.ts 40.74% <0.00%> (-1.86%) ⬇️
src/node/app.ts 69.04% <0.00%> (-1.69%) ⬇️
src/node/util.ts 69.27% <0.00%> (-1.68%) ⬇️
src/node/http.ts 34.92% <0.00%> (-1.15%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbba8e8...5d4e08e. Read the comment docs.

jsjoeio changed the title fix(dev): upgrade to parcel v2 chore(dev): upgrade to parcel v2 Jun 9, 2021
jsjoeio force-pushed the jsjoeio/upgrade-parcel branch 2 times, most recently from 22f4f66 to 60f9ec2 Compare June 9, 2021 21:00
This was referenced Jun 9, 2021
Closed
Merged
jsjoeio commented Jun 9, 2021
View reviewed changes
Comment thread ci/dev/watch.ts Outdated
Comment thread ci/dev/watch.ts Outdated
Comment thread package.json Outdated
Comment thread package.json Outdated
Comment thread package.json Outdated
Comment thread typings/parcel__core/index.d.ts Outdated
jsjoeio force-pushed the jsjoeio/upgrade-parcel branch 2 times, most recently from d7eee9f to 87ed86b Compare June 9, 2021 21:43

jsjoeio commented Jun 9, 2021

Copy link
Copy Markdown
Contributor Author

Currently blocked due to the Build failing due to parcel build in v2 not working the same as in v1.

Filed an issue upstream here but would love any help/suggestions.

jsjoeio force-pushed the jsjoeio/upgrade-parcel branch 2 times, most recently from 2a761a0 to 5623c28 Compare June 10, 2021 23:00
jsjoeio marked this pull request as ready for review June 10, 2021 23:37
jsjoeio requested a review from a team as a code owner June 10, 2021 23:37
jawnsy approved these changes Jun 10, 2021
View reviewed changes

jawnsy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A lot of changes in the deps, it's a good thing we have good test coverage!

Comment thread typings/parcel__core/index.d.ts Outdated
Comment thread yarn.lock
jsjoeio marked this pull request as draft June 11, 2021 21:25

jsjoeio commented Jun 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Turns out this only semi-works.

yarn watch isn't actually working and the end-to-end tests are failing. I'm going to explore the alternative, switching to tsc instead of parcel

jsjoeio commented Jun 12, 2021

Copy link
Copy Markdown
Contributor Author

WIP: https://github.com/cdr/code-server/tree/jsjoeio/remove-parcel

Having issues getting our code to load in the browser after compiling with tsc and then using browserify.

Ideally, it should be easy:

  • take browser-related .ts files
  • compile to .js
  • make browser-compatbile
  • load via <script > tags in .html files

The adventure continues next week.

jsjoeio changed the title chore(dev): upgrade to parcel v2 chore(dev): migrate away from parcel Jun 16, 2021

devongovett commented Jun 20, 2021

Copy link
Copy Markdown

Anything I can help with to upgrade you to Parcel 2? What issues did you run into with it?

jsjoeio mentioned this pull request Jun 21, 2021
Closed
1 task

jsjoeio commented Jun 21, 2021

Copy link
Copy Markdown
Contributor Author

Hey @devongovett - super kind of you to offer to help — thank you!

I wish I took better notes when I was trying to upgrade to Parcel 2. I filed two issues (parcel-bundler/parcel#6433 and parcel-bundler/parcel#6433) which did help but there were other issues we couldn't get past.

All I do remember was something changed between Parcel 1 and Parcel 2 with how TypeScript files were bundled and we couldn't get it working with Parcel 2. There also weren't TypeScript typings, which was a bit of a bummer (but I think that's being worked on? parcel-bundler/parcel#3912)

Regardless, it made us realize that we really only needed to bundle a few files and it was probably overkill asking Parcel to do it for us. We ended up using tsc and browserify and it turned out to be very simple (see migration PR here #3614).

Thanks again for offering to help! Can't tell you how much we appreciate it.

jsjoeio commented Jun 21, 2021
View reviewed changes
Comment thread .gitignore Outdated
Comment thread CHANGELOG.md Outdated
Comment thread package.json Outdated
Comment thread package.json Outdated
Comment thread package.json Outdated
Comment thread test/browser/pages/login.test.ts Outdated
Comment thread test/browser/pages/login.test.ts Outdated
Comment thread test/browser/pages/login.test.ts Outdated
Comment thread typings/parcel__core/index.d.ts Outdated

devongovett commented Jun 21, 2021

Copy link
Copy Markdown

Yes, TS definitions for the API are in progress: parcel-bundler/parcel#6484

Anyway, if you do figure out what the problem was I'm happy to help. Ideally Parcel should work fine for cases like this.

jsjoeio force-pushed the jsjoeio/upgrade-parcel branch from f642631 to 4f40ba9 Compare June 21, 2021 21:56
jsjoeio marked this pull request as ready for review June 21, 2021 22:07
jsjoeio marked this pull request as draft June 21, 2021 22:07
jsjoeio marked this pull request as ready for review June 21, 2021 22:09
jsjoeio force-pushed the jsjoeio/upgrade-parcel branch 2 times, most recently from 735fb5b to ffdb7fc Compare June 22, 2021 18:25
jsjoeio marked this pull request as draft June 22, 2021 18:31
jsjoeio force-pushed the jsjoeio/upgrade-parcel branch 2 times, most recently from 6b1820e to c3eb4e8 Compare June 22, 2021 18:49
jsjoeio marked this pull request as ready for review June 22, 2021 18:52
jsjoeio force-pushed the jsjoeio/upgrade-parcel branch 3 times, most recently from c11c9b0 to db3c921 Compare June 22, 2021 21:43
refactor: remove parcel
5d4e08e 
parent c4e7ee3
author Joe Previte <jjprevite@gmail.com> 1624313323 -0700
committer Joe Previte <jjprevite@gmail.com> 1624386904 -0700
gpgsig -----BEGIN PGP SIGNATURE-----

 iQIzBAABCAAdFiEE6XoH+XX6Zt5LBiiaLJFZDGt0LCQFAmDSLVgACgkQLJFZDGt0
 LCRIsw/8C35RO8Yr/gLs2PDeYXdmfBgB96dj71eZFeUxiURY+6xGkFLWff+Dm7P/
 tN36AQSMkL8Ia8+r5XEQK1q92lQrsJfJKRRJ4OW/Xn2ZF8D0YRZWomhVjKHQRozs
 TPMt3IOeFjmCpF2M8veI3KgbF/p29YTwQ6PcKE2dsodF3CUoEzW9ciswKOinxAev
 iliSgbHuu35hX+OHtAm5ZJh5WOxhr23o9vwuJSLw2+/285sqPPwRO3wwZuLLh6wA
 KYFer3/vfS6+EIs0ushajeeDYsaMYtr55Yc9OI4otcQoT1GQFuGe5D/ISN6XYtmB
 N9RKKkiWVnG959eKQ7ycnDXrt1KQphxkaMR3o3DGsX1FPK6WAxOuDHN+VHZXelLM
 S8ZhmVblbfu8bkkLO8+xb5I1iz4NnV7QDI3q/1dqcq+ZTzRfu0b19RpPSxyZJ5dx
 9g3KFZ29qzovb5aH5+9yQvJlGtflzsL6eIbOYdv7LuslEhE8Ks9Oa2zSjbQ8K/4q
 DAWfj6wR80yg36eL55gH9IJeMT0g+W3oUxRa0Um7PAgNAoFcaBI0jpAPuf1W7Rzd
 2asHTkpENQUHe5fZkUpzVVmkqf2LzhjChAd5cKkdS4pp+ne8sfy6+sJRBoIUmwXy
 zQ8ZYJnsWXLrC/ijiOzFzrTskgr875vj7oEczmjfcyvNutskTG8=
 =kJgg
 -----END PGP SIGNATURE-----

refactor: remove parcel

refactor: upgrade to parcel v2

fix: css-what and normalize-url

refactor: update parcel settings in build-code-server.sh

feat: add .parcel-cache to .gitignore

refactor: reference /src stylesheets in pages/*.html

refactor: remove css imports from register.ts

refactor: bundle pages/*css in release

refactor: upgrade to parcel v2
jsjoeio force-pushed the jsjoeio/upgrade-parcel branch from db3c921 to 5d4e08e Compare June 22, 2021 22:12

jsjoeio commented Jun 22, 2021

Copy link
Copy Markdown
Contributor Author

❗ Current head 684fe6c differs from pull request most recent head 5d4e08e. Consider uploading reports for the commit 5d4e08e to get more accurate results

How to fix:

  1. Get commit (5d4e08e)
  2. Check out locally git checkout 5d4e08e390f14ebdd62a6595a1851897a1377273
  3. Run unit tests to generate output yarn test:unit
  4. Run coverage yarn coverage

Actually, that should work but I'm running into an issue which appears to be upstraem

jsjoeio commented Jun 22, 2021

Copy link
Copy Markdown
Contributor Author

I'm going to try @code-asher's idea tomorrow and create a new branch and PR. I will copy over everything too.

jsjoeio marked this pull request as draft June 22, 2021 23:09

jsjoeio commented Jun 23, 2021

Copy link
Copy Markdown
Contributor Author

Closing in favor of #3658

jsjoeio closed this Jun 23, 2021
jsjoeio mentioned this pull request Jun 23, 2021
Merged
1 task
jsjoeio deleted the jsjoeio/upgrade-parcel branch June 23, 2021 21:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
jawnsy jawnsy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

jsjoeio

Labels

chore Related to maintenance or clean up

Projects

None yet

Milestone

3.11.0

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL