cli icon indicating copy to clipboard operation
cli copied to clipboard

Use pure ES modules

Open ehmicky opened this issue 3 years ago • 10 comments

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

Once https://github.com/netlify/cli/issues/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 avatar Oct 15 '21 14:10 ehmicky

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

ehmicky avatar Nov 22 '21 15:11 ehmicky

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.

ehmicky avatar Nov 23 '21 14:11 ehmicky

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 avatar Nov 25 '21 13:11 lukasholzer

@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

lukasholzer avatar Jan 11 '22 09:01 lukasholzer

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?

eduardoboucas avatar Jan 11 '22 13:01 eduardoboucas

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.

ehmicky avatar Jan 11 '22 14:01 ehmicky

There's a PR for this issue: https://github.com/netlify/cli/pull/4141

benmccann avatar May 07 '22 17:05 benmccann

@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)

lukasholzer avatar May 09 '22 15:05 lukasholzer

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: https://github.com/sveltejs/kit/issues/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

benmccann avatar Sep 01 '22 23:09 benmccann

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

ascorbic avatar Sep 03 '22 08:09 ascorbic

Closed in https://github.com/netlify/cli/pull/6092

sarahetter avatar Nov 17 '23 16:11 sarahetter