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: babel import assertions plugin #2

Merged
merged 3 commits into from
Nov 28, 2022

Conversation

AyAyEm
Copy link
Member

@AyAyEm AyAyEm commented Nov 26, 2022

What did you fix?

Add compatibility to ESM import assertions that node uses


Reproduction steps

  1. Add the @babel/plugin-syntax-import-assertions as dependency
  2. Add it to the esm.js babel plugins section
  3. Change import/extensions in esm.js rules to always

Evidence/screenshot/link to line

Considerations

  • Does this contain a new dependency? [Yes]
  • Does this introduce opinionated data formatting or manual data entry? [Yes
  • Does this pr include updated data files in a separate commit that can be reverted for a clean code-only pr? [No]
  • Have I run the linter? [Yes]
  • Is is a bug fix, feature request, or enhancement? [Feature]

@TobiTenno
Copy link
Member

pretty sure that language feature requires 18

@AyAyEm
Copy link
Member Author

AyAyEm commented Nov 27, 2022

pretty sure that language feature requires 18

that's actually a 16 feature as well https://nodejs.org/docs/latest-v16.x/api/esm.html#json-modules
If i recall correctly the difference would be that it was needed to activate a flag to use it, but that's not a thing anymore

nodejs/node#41736

@TobiTenno
Copy link
Member

that is only in v18+
image

@AyAyEm
Copy link
Member Author

AyAyEm commented Nov 28, 2022

@TobiTenno
Copy link
Member

that still doesn't fix it on v12-14...

so we either remove compatibility with those versions or...?

@AyAyEm
Copy link
Member Author

AyAyEm commented Nov 28, 2022

This pr only changes the esm config, and adds the support for assertions, if you try to use it in v12 it'll just throw an error, or maybe i didn't understand the concern?

@TobiTenno
Copy link
Member

my concern is the ci is literally failing...

@AyAyEm
Copy link
Member Author

AyAyEm commented Nov 28, 2022

there's a CI error it should run npx npm-install-peers instead of npm install it also happened in the other PR
#1

I just runed in v12 in my computer and it seems fine

@TobiTenno TobiTenno mentioned this pull request Nov 28, 2022
@TobiTenno
Copy link
Member

TobiTenno commented Nov 28, 2022

i've put that fix in #3, feel free to approve that and i'll approve this one assuming it still passes after rebase

@TobiTenno
Copy link
Member

i'll reiterate what i mentioned on the other. i'm fine with the language feature being available from our lint config, but given that it's still in experimental support on node 16, i'm not cool with it running a lot of our critical build support

@AyAyEm
Copy link
Member Author

AyAyEm commented Nov 28, 2022

Well it's your choice to close it or merge, i'm fine with any of the options

@TobiTenno TobiTenno enabled auto-merge (squash) November 28, 2022 06:04
@TobiTenno TobiTenno merged commit a450f8a into WFCD:main Nov 28, 2022
@TobiTenno
Copy link
Member

well, shoot, i forgot to ask you to update the docs on the first page for esm

wfcd-bot-boi pushed a commit that referenced this pull request Nov 28, 2022
# [1.3.0](v1.2.0...v1.3.0) (2022-11-28)

### Features

* babel import assertions plugin ([#2](#2)) ([a450f8a](a450f8a))
@wfcd-bot-boi
Copy link

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants