-
Notifications
You must be signed in to change notification settings - Fork 13
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
refactor: Upgrade to parcel v2 and node 19 #260
Conversation
package.json
Outdated
"lint": "prettier --check \"src/**/*.{ts,tsx,js,jsx}\" --ignore-path .gitignore && eslint --ext=.js,.jsx,.ts,.tsx .", | ||
"prepush": "npm run lint" | ||
}, | ||
"author": "k88hudson@gmail.com", | ||
"license": "MIT", | ||
"dependencies": { | ||
"body-parser": "1.19.0", | ||
"body-parser": "^1.20.2", | ||
"express": "4.17.3", |
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 think we can upgrade express, request, and maybe some others. I initially upgraded everything and then just backpedaled from there until it mostly worked. But I think the issue I downgraded these things over was actually resolved by the dirName
hack in server.ts.
For future reference, the issue there was that parcel v2 now does this ridiculous thing where it appends a string to path.resolve(__dirname)
to traverse from the dist folder to the src folder. It seems like this is supposed to happen just for browser targets, and then it's supposed to be evaluated and replaced with the return value.
But in this case, it did the replacing in a node target, and then didn't evaluate it. So I was left with path.resolve(__dirname, "../src")
in server.js which is supposed to be run in node. Which causes the server to serve the src files instead of the dist files 😂. I had to basically trick it into not noticing it, by doing path.resolve(eval("__dirname"))
which is just...
This also fixes the regression from a couple months ago where the content bundle ends up importing server code. I would also like to fix the reverse issue, where the server bundle imports content code. But that would better be resolved in whatever PR we come up with that deals more broadly with organizing our typings. Anyway, this is a pretty colossal change - it's just not possible to make incremental improvements here since parcel v1 to v2 is a total paradigm shift. It forces a lot of changes. I would appreciate any help testing this, so I don't miss any subtle issues. We have to rename all the CSS modules because the alternative is to parse ALL stylesheets as modules, which breaks index.scss. With no way to exclude individual stylesheets from that behavior. Another alternative would be importing index.scss in index.tsx, but I just generally don't like the idea of precluding global stylesheets. |
package.json
Outdated
"@parcel/transformer-css": "^2.8.3", | ||
"@parcel/transformer-sass": "^2.8.3", | ||
"@types/body-parser": "^1.19.2", | ||
"@types/express": "^4.17.17", | ||
"@types/luxon": "1.21.0", |
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.
Check if there's an upgrade for this too
Closes #211