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

switch laravel mix to vite #423

Merged
merged 11 commits into from
Oct 19, 2024
Merged

switch laravel mix to vite #423

merged 11 commits into from
Oct 19, 2024

Conversation

joshreisner
Copy link
Contributor

@joshreisner joshreisner commented Oct 7, 2024

creating this as a draft PR of three issues, and i don't know enough yet about cjs vs es modules to solve them. the issues are:

  1. shipping this would require us to add type="module" to everyone's app scripts

  2. also npm run jest doesn't run because of the error:

ReferenceError: module is not defined in ES module scope
This file is being treated as an ES module because it has a '.js' file extension and 'tsml-ui/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
  1. finally, it's creating an extra app script for mapbox-gl in dist/assets/ which is not ideal -- not sure what to do about that

@joshreisner joshreisner requested a review from gkovats October 7, 2024 00:03
@joshreisner joshreisner linked an issue Oct 7, 2024 that may be closed by this pull request
Copy link

netlify bot commented Oct 7, 2024

Deploy Preview for tsml-ui ready!

Name Link
🔨 Latest commit a4df76b
🔍 Latest deploy log https://app.netlify.com/sites/tsml-ui/deploys/67094620aab2ff00084a4e65
😎 Deploy Preview https://deploy-preview-423--tsml-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@joshreisner joshreisner changed the title switching laravel mix to vite switch laravel mix to vite Oct 10, 2024
@@ -53,6 +53,6 @@
: {},
};
</script>
<script src="/app.js" async></script>
<script src="/src/app.tsx" type="module" async></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just the example - the actual embed codes haven't changed

Lang: 'readonly',
TSMLReactConfig: 'readonly',
Translation: 'readonly',
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i could not get jest to recognize the global types defined in index.d.ts so they're no longer global - also TSMLReactConfig got renamed to Settings

"react": "^18.2.0",
"react-dom": "^18.2.0",
"luxon": "^3.5.0",
"mapbox-gl": "^3.7.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.

it complained that this was missing - we were referencing files in it - so now it's an explicit dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i removed all these test files in favor of a single example.html - i think this is more vite-y

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure?

...state.input[filter]
.map(key => getIndexByKey(state.indexes[filter], key)?.slugs ?? [])
.flat(),
]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to fix all the // @ts-expect-errors for jest - these places are the only places where syntax changes. i tested this of course but if there's a regression it's in one of these places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fun fact we weren't using this file

@joshreisner joshreisner marked this pull request as ready for review October 11, 2024 15:22
@joshreisner joshreisner self-assigned this Oct 11, 2024
@joshreisner
Copy link
Contributor Author

i've switched out https://aasanjose.org/meetings to use the new app bundle

inlineDynamicImports: true,
},
},
target: 'es2021',
Copy link
Collaborator

Choose a reason for hiding this comment

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

@joshreisner when I add here:
copyPublicDir: false,
It doesn't copy logo.svg and a few pointless files in /dist folder. Was just poking at various config options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - logo.svg is needed though for the https://tsml-ui.code4recovery.org/ homepage 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh! This is the public page, neat, makes sense.

@@ -0,0 +1 @@
{"root":["./src/app.tsx","./src/vite-env.d.ts","./src/components/alert.tsx","./src/components/button.tsx","./src/components/controls.tsx","./src/components/dropdown.tsx","./src/components/icon.tsx","./src/components/link.tsx","./src/components/loading.tsx","./src/components/map.tsx","./src/components/meeting.tsx","./src/components/table.tsx","./src/components/title.tsx","./src/components/tsmlui.tsx","./src/components/index.ts","./src/helpers/analytics.ts","./src/helpers/calculate-distances.ts","./src/helpers/filter-meeting-data.ts","./src/helpers/flatten-and-sort-indexes.ts","./src/helpers/format-address.ts","./src/helpers/format-array.ts","./src/helpers/format-conference-provider.ts","./src/helpers/format-directions-url.ts","./src/helpers/format-feedback-email.ts","./src/helpers/format-ics.ts","./src/helpers/format-slug.ts","./src/helpers/format-string.ts","./src/helpers/format-url.ts","./src/helpers/get-index-by-key.ts","./src/helpers/index.ts","./src/helpers/load-meeting-data.ts","./src/helpers/query-string.ts","./src/helpers/settings.ts","./src/helpers/translate-google-sheet.ts","./src/helpers/user-agent.ts","./src/i18n/en.ts","./src/i18n/es.ts","./src/i18n/fr.ts","./src/i18n/index.ts","./src/i18n/ja.ts","./src/i18n/nl.ts","./src/i18n/pt.ts","./src/i18n/sk.ts","./src/i18n/sv.ts","./src/styles/alert.ts","./src/styles/button.ts","./src/styles/controls.ts","./src/styles/dropdown.ts","./src/styles/global.ts","./src/styles/index.ts","./src/styles/loading.ts","./src/styles/map.ts","./src/styles/meeting.ts","./src/styles/table.ts","./src/styles/variables.ts","./src/types/jsondata.ts","./src/types/meeting.ts","./src/types/metatype.ts","./src/types/settings.ts","./src/types/state.ts","./src/types/translation.ts","./src/types/index.ts"],"version":"5.6.2"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I build this locally, this file changes - /src/components/alert.tsx becomes /src/components/Alert.tsx, etc. Case of files match local file paths. Not sure if this is a difference between iOS and Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh, it might be mac vs linux! we can keep an eye on that - i'd kind of prefer this file to use the correct casing

Copy link
Collaborator

@gkovats gkovats left a comment

Choose a reason for hiding this comment

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

@joshreisner Clearly it builds and works. Congrats on getting it to work! I know Vite is the new big build tool, it's opinionated and simplifies stuff. Seems nice, not having to run 2 commands to see locally. I want to update /example.html shortly to allow for multiple test meetings, or we'd just change it to test production issues as needed and revert before commit.

Like I mentioned in chat, only things I saw:

  • adds 400kb to size, but there's a lot of polyfill voodoo a build tool adds, figure it's in that.
  • I don't know why it requires async on my local, but that's a concern if we make this version live. Don't know if you can make every user also update TSML, or folks that have one-off html embeds. Maybe we could offer some helper JS that just embeds the latest script tag necessary for TSML-UI?
  • looking at the size, it just prompted me to want to try another map library, just to possibly trim down the size of the widget. I know, bandwidth is cheap, gzip, and caching and all, but some folks are still stuck on 3G, so little things help.

@joshreisner
Copy link
Contributor Author

yeah, shifting over to es modules would probably help too, since that lib file would get cached, i think.

having our users switch away from mapbox to a different service might be a headache, if there's a way to avoid that. or maybe we might look at other mapbox configurations, maybe we don't need all these dependencies?

i haven't been able to repro this async issue but it might make sense to release another version of TSML with that option in it so if users run into problems they can always update

@joshreisner joshreisner merged commit cb53072 into main Oct 19, 2024
6 checks passed
joshreisner added a commit that referenced this pull request Nov 13, 2024
joshreisner added a commit that referenced this pull request Nov 15, 2024
* Revert "use terser to fix #426 (#427)"

This reverts commit 4c80c4e.

* Revert "switch laravel mix to vite (#423)"

This reverts commit cb53072.
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.

switch to vite
2 participants