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

Webpack 4 example #53

Merged
merged 10 commits into from
Jan 11, 2024
Merged

Webpack 4 example #53

merged 10 commits into from
Jan 11, 2024

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Jan 9, 2024

This PR adds a separate example for Webpack version 4 alongside Webpack 5. Ideally most people should be on version 5 as soon as possible and on all new products. However, older projects or ones using older versions of frameworks like Nuxt2 and older Angular versions still rely on Webpack version 4

There are some challenges wrapped in getting this to work for Webpack 4 which mostly stem from using newer JS features that Webpack doesn't know how to support.

  • Requires babel for transformations like optional chaining (.?)
  • Requires a new loader to let Webpack understand import.meta (@open-wc/webpack-import-meta-loader)
    • This seems not ideal and definitely seems to increase build time, may want to look into an alternative
  • Requires ignoring "critical" errors about not being able to fully statically analyze cesium's code
    • Speaking with @ggetz this is unavoidable with the way cesium needs to be structured. However Webpack 5 doesn't require any extra config for this which is very curious to me

I also updated the github actions to make sure to perform the build checks and linting against both webpack 4 and 5 versions.

Testing

  • Change to both the webpack-4 and webpack-5 directories
  • Run npm install
  • Run npm run build
  • Run npm start and make sure the page works correctly at http://localhost:8080/
  • Run npm run start:built and make sure the page works correctly at http://localhost:8080/

There should not be any errors in the console or on the page for both version 4 and 5

@jjspace jjspace requested a review from ggetz January 9, 2024 18:25
@jjspace
Copy link
Contributor Author

jjspace commented Jan 9, 2024

@ggetz I wasn't totally sure the best place to do linting so right now it's in each directory as a "complete package" but it may make more sense to still initialize the whole project as a npm project and do it only at the top level? I haven't worked with npm workspaces or anything but would that make more sense here?

I also realized that this method may not work with husky? Thoughts?

@jjspace
Copy link
Contributor Author

jjspace commented Jan 10, 2024

@ggetz I restructured the packages a little pulling eslint, prettier and husky up to the top level as discussed this morning. I tried using the npm workspaces but realized that it's optimizations for package install locations make the examples trickier, specifically the "copy static files" step. I decided to not use workspaces in favor of having each example stand on it's own and work in isolation to allow people to copy it into their own project easier.

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @jjspace! The new example is working well 👍 I think it makes sense to continue with this approach.

.vscode/settings.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
webpack-4/package.json Outdated Show resolved Hide resolved
webpack-4/webpack.config.js Outdated Show resolved Hide resolved
webpack-4/webpack.config.js Outdated Show resolved Hide resolved
webpack-4/webpack.config.js Outdated Show resolved Hide resolved
webpack-5/README.md Outdated Show resolved Hide resolved
@jjspace
Copy link
Contributor Author

jjspace commented Jan 11, 2024

Thanks for the comments @ggetz, I've addressed or responded to them all, please take another look

README.md Outdated Show resolved Hide resolved
@jjspace
Copy link
Contributor Author

jjspace commented Jan 11, 2024

@ggetz updated again, I reorganized the main readme to avoid confusion

@ggetz
Copy link
Contributor

ggetz commented Jan 11, 2024

Awesome. Thanks @jjspace!

@ggetz ggetz merged commit 6246e37 into main Jan 11, 2024
5 checks passed
@ggetz ggetz deleted the webpack-4-example branch January 11, 2024 21:04
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