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

General fixes before releasing #1974

Merged
merged 9 commits into from
Apr 11, 2024
Merged

General fixes before releasing #1974

merged 9 commits into from
Apr 11, 2024

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Apr 11, 2024

Check the commits separately.

This comment was marked as resolved.

This comment was marked as off-topic.

Comment on lines +1002 to +1003
if (isPrerelease || /^[a-z]+$/i.test(currentVersion)) {
// Skip prereleases or versions like 'next' or 'latest'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using next as the version in package.json made this function throw cc @juanpprieto

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be really nice to make it work with the next release. Even if we don't have all the fancy ui with what's changed. Just making it easy to upgrade an existing project to next seems to be a good idea.

Comment on lines -160 to +162
const eslintrcPath = joinPath(projectDir, '.eslintrc.js');
let eslintrc = await readFile(eslintrcPath);
const {filepath = joinPath(projectDir, '.eslintrc.cjs')} =
await findFileWithExtension(projectDir, '.eslintrc', ['cjs', 'js']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are now using .cjs extension in Vite projects because they are "type": "module".

We should update to eslint@9 eventually and start using the new convention (eslint.config.js) but I'm not sure if the rest of the Shopify eslint configs we have are compatible.

@@ -66,7 +66,8 @@
"@shopify/hydrogen-react": "2024.4.0",
"content-security-policy-builder": "^2.1.1",
"type-fest": "^4.5.0",
"source-map-support": "^0.5.21"
"source-map-support": "^0.5.21",
"use-resize-observer": "^9.1.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunate but we need to add this dependency for /subrequest-profiler to work here, since the virtual routes are now provided in the Hydrogen package.

We should make a PR to this package and have them publish an UMD version so that we don't need to do this...

@frandiox frandiox requested a review from a team April 11, 2024 12:42
@frandiox frandiox changed the title Release fixes General fixes before releasing Apr 11, 2024
Comment on lines +73 to +75
optimizeDeps: {
include: ['@shopify/hydrogen'],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll be honest and I don't quite understand what's going on here. It seems vite automatically recognizes the mono repo and does not pre-bundle @shopify/hydrogen dependencies. include forces them to be pre-bundled. How does that affect initial reload?

Copy link
Contributor Author

@frandiox frandiox Apr 11, 2024

Choose a reason for hiding this comment

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

Vite decides to optimize Hydrogen (I'm not really sure why) when it's used outside of our monorepo. You can see this in the demo-store:

image

This just lets Vite find the dependency and optimize it earlier to avoid the initial refresh.

Comment on lines 5 to 7
public-hoist-pattern[]=cookie
public-hoist-pattern[]=set-cookie-parser
public-hoist-pattern[]=content-security-policy-builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading about public-hoist-pattern and I'm curious to know the error you encountered to know that these needed to get hoisted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's this issue: #1834 (comment)

Vite can't find cookie to optimize it even if we tell it to do it. The reason is that if you check node_modules/cookie, it's not there in PNPM. It is in node_modules/.pnpm/node_modules/cookie instead, but Vite doesn't know that.

This change forces pnpm to hoist cookie to node_modules/cookie so that Vite can find it.

@frandiox frandiox merged commit 052eb14 into main Apr 11, 2024
13 checks passed
@frandiox frandiox deleted the fd-release-fixes branch April 11, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants