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

build: update merger filename and revert @nuxt/content upgrade #867

Merged
merged 9 commits into from
Jun 30, 2024

Conversation

danielroe
Copy link
Contributor

No description provided.

@danielroe danielroe requested a review from ineshbose June 28, 2024 15:58
@danielroe danielroe self-assigned this Jun 28, 2024
Copy link

what-the-diff bot commented Jun 28, 2024

PR Summary

  • Introduction of a New Component
    We've introduced a new component to the software. This addition is expected to greatly enhance overall application functionality and user experience.

Copy link

netlify bot commented Jun 28, 2024

Deploy Preview for nuxt-tailwindcss ready!

Name Link
🔨 Latest commit 2fe53af
🔍 Latest deploy log https://app.netlify.com/sites/nuxt-tailwindcss/deploys/668071a3dac08f0008e38930
😎 Deploy Preview https://deploy-preview-867--nuxt-tailwindcss.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ineshbose
Copy link
Collaborator

Thanks for raising this PR but that TODO is what I have been wondering about, as the whole PostCSS/config may be in CJS syntax and/or Nuxt 2 projects may be in CJS. The MJS is explicit and helpful no? What shortcomings were there specifying that extension?

@danielroe
Copy link
Contributor Author

danielroe commented Jun 28, 2024

The error is not coming from loading the merger.js file. Even when I update it to be .mjs, the same warnings are logged. I would not be surprised if it's a product of jiti loading tailwindcss or something similar. I believe you can reproduce with a fresh project using @nuxtjs/tailwindcss.

The main shortcoming is that *.mjs files require *.d.mts declarations for new TS bundler resolution as well as *.d.ts declarations for old TS resolution, meaning we have to emit twice as many files for backwards compatibility.

Also, this library is type: module so Node will always treat .js files in the project as ESM anyway.

And runtime/ files are mainly meant to be loaded by the build-time context which is always going to resolve them as ESM.

I would also add that supporting Nuxt 2 (with default behaviour) isn't a priority as it is EOL in a couple of days, though of course you can always revert the default extension to .mjs if you think it is important for this module.

@danielroe
Copy link
Contributor Author

danielroe commented Jun 29, 2024

@ineshbose Seems we have a regression somewhere in that deps upgrade (though not, apparently, in the module-builder update)...

If you want to confirm that the .js extension isn't the cause of it, feel free to use the following build.config.ts (and rename merger.js -> merger.mjs):

import { defineBuildConfig } from 'unbuild'

export default defineBuildConfig({
  hooks: {
    'mkdist:entries'(ctx, entries) {
      entries[0].ext = 'mjs'
    }
  }
})

@danielroe danielroe changed the title build: update merger filename build: update merger filename and revert some deps upgrade Jun 29, 2024
@danielroe danielroe changed the title build: update merger filename and revert some deps upgrade build: update merger filename and revert @nuxt/content upgrade Jun 29, 2024
Copy link
Collaborator

@ineshbose ineshbose left a comment

Choose a reason for hiding this comment

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

Thank you ❤️

@ineshbose ineshbose merged commit bb1b78b into main Jun 30, 2024
6 checks passed
@ineshbose ineshbose deleted the build/merger branch June 30, 2024 14:01
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

2 participants