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: add cjs and mjs support for babel #2334

Merged

Conversation

xiaoxiangmoe
Copy link
Contributor

Fixes #IssueNumber

Changes proposed:

  • Add
  • Delete
  • Fix
  • Prepare

see babel/babel#10599 and babel/babel#10903

@JimiC
Copy link
Member

JimiC commented Jan 12, 2020

This definitely needs to be reduced using globExtensions and globFilenames,

@xiaoxiangmoe
Copy link
Contributor Author

@JimiC Should we remove 'babel.config.esm.js'? I only find 'babel.config.mjs' in babel's release note.

related pr: #2319

@JimiC
Copy link
Member

JimiC commented Jan 12, 2020

@robertohuertasm knows something about it I suppose.

@KingDarBoja
Copy link
Member

I was looking at the docs and babel.config.esm.jsdoesn't exist anymore.

@JimiC
Copy link
Member

JimiC commented Jan 12, 2020

Maybe keep esm for backwards compatibility. Some may still use it.

@xiaoxiangmoe
Copy link
Contributor Author

xiaoxiangmoe commented Jan 12, 2020

@JimiC @KingDarBoja I don't think we should keep esm for backwards compatibility. Icon does not break existing code. The invalidation of the icon will remind them to update the filename.

@JimiC
Copy link
Member

JimiC commented Jan 12, 2020

@xiaoxiangmoe If we only lived in a perfect world...

@xiaoxiangmoe
Copy link
Contributor Author

xiaoxiangmoe commented Jan 12, 2020

@KingDarBoja @JimiC @ManzDev I can't find 'babel.config.esm.js' in any version of babel docs.


git clone https://github.com/babel/website.git 
cd website
git grep 'babel\.config\.mjs' $(git rev-list --all) > mjs-docs
git grep 'babel\.config\.esm\.js' $(git rev-list --all) > esm-docs
# wait for a long time
cat esm-docs # empty
cat mjs-docs # 9 line contents

#2319 also introduced gulpfile.esm.js, and I found description in https://gulpjs.com/docs/en/getting-started/javascript-and-gulpfiles

@JimiC
Copy link
Member

JimiC commented Jan 12, 2020

Ask @ManzDev.

JimiC
JimiC previously requested changes Jan 12, 2020
src/iconsManifest/supportedExtensions.ts Outdated Show resolved Hide resolved
Copy link
Member

@JimiC JimiC left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@robertohuertasm robertohuertasm merged commit 642a4ae into vscode-icons:master Feb 14, 2020
@robertohuertasm
Copy link
Member

@xiaoxiangmoe I've received a notification from GitHub of a message from you that I cannot find anymore asking for updates (probably you deleted it). Thanks for this PR and the rest of the team for their supervision. It's merged now! 🎉

@robertohuertasm robertohuertasm added this to the Next milestone Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants