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 pure ES modules #3514

Closed
ehmicky opened this issue Oct 15, 2021 · 11 comments
Closed

Use pure ES modules #3514

ehmicky opened this issue Oct 15, 2021 · 11 comments
Labels
type: chore work needed to keep the product and development running smoothly

Comments

@ehmicky
Copy link
Contributor

ehmicky commented Oct 15, 2021

See background at https://github.com/netlify/team-dev/issues/36

Once #3512 is done and released, we should use pure ES modules and make a major release.

This is more than just switching from CommonJS to import/export. See this list for other changes which might be involved. This should be broken in many PRs, as much as possible, to lower the risk. Also, non-breaking changes (such as adding file extensions in imports, or loading JSON files differently) should be done before the breaking changes (such as using import/export statements).

Since we can make a major release, upgrading would require a user action and no sites would be broken.

@ehmicky
Copy link
Contributor Author

ehmicky commented Nov 22, 2021

This was discussed in our guild sync and @lukasholzer decided to start moving on this item.

@ehmicky
Copy link
Contributor Author

ehmicky commented Nov 23, 2021

After discussing with @lukasholzer and @eduardoboucas, we decided on the following plan:

  • 1. Adding .ts file extensions to all files and import/export. Without types yet, nor additional tooling (except for tsc) or re-architecturing.
  • 2. Move to pure ES modules.
  • 3. Complete the TypeScript migration, by adding types, etc.

@lukasholzer
Copy link
Collaborator

As going further with the migration it seems that we are blocked by oclif therefore I suggest implementing this issue earlier: https://github.com/netlify/pod-workflow/issues/343 (internal issue)

@lukasholzer
Copy link
Collaborator

@ehmicky, @erezrokah I hit some further roadblocks on the cli repository I wanted to discuss with the group.

The CLI is using the require cache mechanism to serve a clean state of the functions locally when developing. When we switch to es modules this feature is not available anymore.

So I'm asking you about ideas on how we can solve that. Some options came to my mind.

  1. Migrate to dynamic imports without any cache clearing just plain imports. (less preferred)
  2. Always bundle locally with esbuild to have the dependencies inside the file, make esbuild default and use it's serve mode. (No problem with dependency caching at all)

Please add your opinions on that one. cc @eduardoboucas

@eduardoboucas
Copy link
Member

Always bundle locally with esbuild to have the dependencies inside the file, make esbuild default and use it's serve mode. (No problem with dependency caching at all)

We should do everything we can to make sure the local development experience mimics the production environment as much as possible. esbuild is not the default bundler in production, and it's unlikely it will be anytime soon, so we shouldn't use it in the CLI unless people opt into it.

I know that we had some issues in the past with the cache mechanism when serving functions, with paths that should be updated after a file save but weren't.

If this functionality is only available in CJS, can we have a CJS file that's included by the remaining ES codebase? And if that's not an option, will that not be a problem when we load CJS functions defined by users?

@ehmicky
Copy link
Contributor Author

ehmicky commented Jan 11, 2022

When it comes to mimicking require.cache with pure ES modules, I believe dynamic imports can be used with a cache busting query parameter.

With CommonJS:

require('/path/to/example.js') // Cache not used
require('/path/to/example.js') // Cache used
delete require.cache['/path/to/example.js']
require('/path/to/example.js') // Cache not used

With pure ES modules (note: this only works if the importing file is using pure ES modules):

await import('/path/to/example.js?one') // Cache not used
await import('/path/to/example.js?one') // Cache used
await import('/path/to/example.js?two') // Cache not used

Please let me know if my comment is out of topic, as I do not know how caching is currently implemented with functions served locally.

@benmccann
Copy link

There's a PR for this issue: #4141

@lukasholzer
Copy link
Collaborator

@benmccann this PR is only the top of the iceberg. There is a lot of work for the functions resolving to make it compatible. (as some require functionality is used there)

@benmccann
Copy link

SvelteKit deploys ESM-only on all platforms except Netlify. The lack of ESM support on Netlify is a continual pain point because SvelteKit and Vite need to bundle to CJS just to support Netlify. Things are more likely to break for Netlify users than users of other platforms since Netlify is the odd-man out in this situation. We currently have a bug caused by where the latest version of SvelteKit is broken for Netlify: sveltejs/kit#6462. I'd really love to see this addressed at some point.

FYI @ascorbic - thought you might be interested in this as well since you've contributed to SvelteKit on behalf of Netlify. Thanks again for the help

@ascorbic
Copy link
Contributor

ascorbic commented Sep 3, 2022

Hi @benmccann
Thanks for the nudge on this. I don't think this is related to this issue (this about rewriting the CLI itself in ESM). However I am going to look into this for you further. I know it's soemthing that was being worked on, but I'm not sure of the current status.
The approach that Nuxt takes is to build to ESM, but have a cjs entrypoint which does a dynamic import for the real ESM bundle. Would that work for SvelteKit? https://github.com/unjs/nitro/blob/main/src/presets/netlify.ts#L36-L44

@sarahetter
Copy link
Contributor

Closed in #6092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants