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

Use babel node for local development to support ES6 import #1278

Merged
merged 5 commits into from
Jun 20, 2023

Conversation

kevinxh
Copy link
Collaborator

@kevinxh kevinxh commented Jun 19, 2023

Description

In PWA Kit V2/V3, the server side code ssr.js (or any files imported from ssr.js) are Commonjs modules. However, in V1, ES6 import statements were supported. Some customers encountered difficulties upgrading from v1 to v2 because of the non-compatible changes introduced in V2.

This PR explores the options to bring back ES6 import statement to ssr.js.

TODO:

  • ensure changes work on MRT remote environment

Rollout plan

  • patch v2.7
  • patch v3.0
  • merge this into develop so v3.1 and onwards have it

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • use @bable/node for pwa-kit-dev start

How to Test-Drive This PR

  • pull branch
  • npm ci
  • cd packages/template-retail-react-app
  • npm start
  • app starts up without issue

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@kevinxh kevinxh requested a review from a team as a code owner June 19, 2023 21:47
execSync(`node${inspect ? ' --inspect' : ''} ${resolvedSSRPath}`, {
// We use @babel/node instead of node because we want to support ES6 import syntax
const babelNode = p.join(
require.resolve('webpack'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why webpack rather than babel-node itself?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because for some reason, babel packages have weird namings, i think babel node was called babel-node and since a major version (i forgot exactly which version), they moved to scoped packages like @babel/node.

However, require.resolve doesn't work for both babel-node nor @babel/node, so i had to fall back to webpack...

I also don't understand why all pwa-kit-dev commands resolves to bin using path.join('..','..','..'), but i just followed the existing pattern...

if anyone knows the reason, pls let me know.

'.bin',
'babel-node'
)
execSync(`${babelNode} ${inspect ? '--inspect' : ''} ${resolvedSSRPath}`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just npx babel-node?

Copy link
Collaborator Author

@kevinxh kevinxh Jun 20, 2023

Choose a reason for hiding this comment

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

i don't think require.resolve finds the binaries from pwa-kit-dev correctly, and that causes a delay for the start command when you run it the first time, cuz npx will try to install it...

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.

5 participants