-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(remix): Add Remix 2.x release support. #8940
Conversation
6efcf2c
to
0149461
Compare
size-limit report 📦
|
abb124e
to
b6ec1a3
Compare
aef6350
to
ff4d574
Compare
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.
Can we open up a separate PR for the node version changes on the e2e test? That way this PR can only look at changes to remix.
.github/workflows/build.yml
Outdated
@@ -818,7 +819,7 @@ jobs: | |||
- name: Set up Node | |||
uses: actions/setup-node@v3 | |||
with: | |||
node-version-file: 'package.json' | |||
node-version: 18 |
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.
Can we read the e2e-tests package.json here instead?
9b935d0
to
67c3c4e
Compare
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.
One last thing before we merge - is there no way to get the remix version automatically? I would prefer not forcing folks to use the isRemixV2
flag.
Update: Passed remix version from server |
e62e166
to
09a1425
Compare
5eb3369
to
2ab1c11
Compare
Just FYI after upgrading my own setup:
|
const pkg = loadModule<{ version: string }>('@remix-run/react/package.json'); | ||
const version = pkg ? pkg.version : '0.0.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.
m: If we stick with this approach (see my other comment), let's add a debug log if we fail to load package.json
081e762
to
88a7f23
Compare
Remix V2 is out, would love to get this in 😅 |
5917bf9
to
d6badeb
Compare
Thanks for the trial @huw, it's been very helpful!
I was not able to reproduce this, but will take another look after the release.
This may need a bit of discussion and new test cases. We can revisit these two points as improvements in other PRs. The other 3 points @huw reported are now included in the first version. |
d6badeb
to
7164807
Compare
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.
LGTM now! I agree with merging this in now and tackling the open questions in follow up tasks/PRs. Thanks Onur!
packages/e2e-tests/test-applications/create-remix-app-v2/app/entry.client.tsx
Outdated
Show resolved
Hide resolved
packages/e2e-tests/test-applications/create-remix-app-v2/playwright.config.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Lukas Stracke <[email protected]>
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.
Let's ship this as a first step - thanks Onur!
Resolves: #8681
closes #9040
Summary:
18.x
. (This is required for Remix v2.x, and 18.x is the current LTS so updated this for all) chore(e2e-tests): Use Node 18 for E2E tests. #8964create-remix-app
E2E tests to cover client-side events and transactions.create-remix-app-v2
E2E test application that uses2.0.0
version.Events:
pageload
transaction - Linknavigation
transaction - Link