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

feat: move from CJS to ESM #2566

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Nov 21, 2023

this is an unreasonably large change which does a few things:

  • Moves all packages to type: "module"
  • Removes node-fetch (ran into problems in ESM-land)
  • Replaces all __dirname with an equivalent using import.meta
  • Replaces all require.resolve with import.meta.resolve
  • Update all imports to be ESM-compatible
  • Move all tsconfig to use nodenext resolution

Probably some other stuff too.

@koddsson if your branch is cleaner, and easier to review etc etc just let me know. im happy to throw this away, it was still a learning experience and i did go upstream to fix a few dependencies we have to make them work in nodenext.

ah yeah and it'll be outdated, probably a truckload of conflicts but i can update if we want it

Copy link

changeset-bot bot commented Nov 21, 2023

⚠️ No Changeset found

Latest commit: aa809a7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

this is an unreasonably large change which does a few things:

- Moves all packages to `type: "module"`
- Removes `node-fetch` (ran into problems in ESM-land)
- Replaces all `__dirname` with an equivalent using `import.meta`
- Replaces all `require.resolve` with `import.meta.resolve`
- Update all imports to be ESM-compatible
- Move all `tsconfig` to use `nodenext` resolution

Probably some other stuff too.
@koddsson
Copy link
Contributor

I think a good way forward is to create a next branch and push my PR to that branch and make a prerelease. Then we can rebase this PR against the next branch which makes it easier to look at the diff of your changes and my changes and get this PR running green and then merge this and possible other PRs to next and keep releasing prereleases until we're happy and do a actual major release.

@koddsson
Copy link
Contributor

Actually I forgot I just commented out some tests in that PR. I gotta get those fixed first.

@koddsson
Copy link
Contributor

Actually I forgot I just commented out some tests in that PR. I gotta get those fixed first.

Ended up fixing this. I've pushed my PR to the next branch where the action has published all those packages as canary packages to npm so we can start testing those out.

If you rebase this PR against next we can look at the diff and start pulling relevant parts into the next branch and therefore the canary npm tag.

@43081j
Copy link
Contributor Author

43081j commented Nov 22, 2023

i haven't had chance to look yet what you've done. it sounds like you've done a lot of what i've done here, but the shorter route (e.g. createRequire).

i went all in on this branch instead so its probably less likely to land. for example:

  • all fake paths have been removed (the aliases in export maps which don't actually exist, i.e. you now have to import @web/whatever/dist/whatever.js rather than @web/whatever/whatever.js)
  • all CJS support has been dropped (from export maps, builds, etc)
  • upgraded a few dependencies to make them happy in nodenext (not sure how you managed to get away with not doing this, as i even had to fix some upstream you definitely don't have)

e.g. there's also some type errors in rollup's plugins in nodenext which you can't have worked around as they're broken at the source. so i'd be curious how you did that

maybe you haven't had to tackle nodenext? and you're still using the older resolution strategy?

either way, i don't think you can just hope to rebase this branch onto yours. you first need to fully understand whats in your branch vs whats in my branch, so we know what exactly we're trying to take from each IMO.

@bashmish
Copy link
Member

@koddsson where shall I post bugs found in the canary release?

@koddsson
Copy link
Contributor

@koddsson where shall I post bugs found in the canary release?

I think just a issue would be great :)

@bashmish
Copy link
Member

@koddsson where shall I post bugs found in the canary release?

I think just a issue would be great :)

What is your plan in releasing this? A separate issue I think is better for what is already released which is not the case here.
Currently the storybook-builder is broken, I'd like to prevent a release of it with a broken version. IIRC you also wanted to bump a major version here, maybe even bump all 0.x.x to 1.x.x, I.m not sure it's a good idea for storybook-builder either.

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.

None yet

3 participants