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(webpack): allow function entries for build.transpile #6120

Merged
merged 8 commits into from
Aug 3, 2019

Conversation

atinux
Copy link
Member

@atinux atinux commented Jul 24, 2019

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

It's mostly a feature for module, thinking of @nuxt/http for example that need to add ky in build.transpile but only for legacy mode.

Before:

{
  build: {
    transpile: ['ky']
  }
}

After:

{
  build: {
    transpile: [
      ({ isModern }) => isModern ? null : 'ky'
    ]
  }
}

The main issue I see is that module author will have to check Nuxt version to be able to push a function in build.transpile otherwise it justs breaks 😢

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: v2.9.0 docs#1472)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@atinux atinux requested review from clarkdo, pi0 and a team July 24, 2019 10:28
@pi0 pi0 changed the title feat: handle build.transpile item to be a function feat(webpack): allow function entries for build.transpile Jul 24, 2019
pi0
pi0 previously approved these changes Jul 24, 2019
@pi0
Copy link
Member

pi0 commented Jul 24, 2019

The main issue I see is that module author will have to check Nuxt version to be able to push a function in build.transpile otherwise it justs breaks cry

This is a feat/fix by next release. Nothing will be breaking.


Some other enhancements we could do is adding an option like transpileLegacy or allow object form like { name: 'ky', legacy: true } or { name: 'ky', modern: false }

galvez
galvez previously approved these changes Jul 24, 2019
@atinux atinux dismissed stale reviews from galvez and pi0 via 89d9876 July 24, 2019 10:42
@atinux atinux mentioned this pull request Jul 24, 2019
@codecov-io
Copy link

codecov-io commented Jul 24, 2019

Codecov Report

Merging #6120 into dev will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6120      +/-   ##
==========================================
+ Coverage   95.75%   95.75%   +<.01%     
==========================================
  Files          80       80              
  Lines        2659     2665       +6     
  Branches      682      686       +4     
==========================================
+ Hits         2546     2552       +6     
  Misses         97       97              
  Partials       16       16
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.91% <75%> (+0.03%) ⬆️
#unit 92.45% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
packages/webpack/src/config/base.js 94.89% <100%> (+0.16%) ⬆️
packages/webpack/src/config/server.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edf5e66...63dbd61. Read the comment docs.

Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

If function is one of array items, then it can only return string or regex pattern.

Do you think is there any use case for multiple functions, maybe modules ?

How about just making transpile as function, like:

transpile: () => {
  return [
    'vue-test',
    '@vue/test'
  ]
}

if (pattern instanceof RegExp) {
items.push(pattern)
} else {
} else if (pattern) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be string ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, Will update to check typeof pattern === 'string'

@atinux
Copy link
Member Author

atinux commented Jul 29, 2019

@clarkdo Making build.transpile a function will be harder for modules author to extend it I guess, this is why I thought about handling a push as a method (see nuxt/http@2ce08de#diff-d1234a869b3d648ebfcdce5a76747d71R100)

@atinux
Copy link
Member Author

atinux commented Jul 29, 2019

Ready to review

@atinux atinux requested review from pi0, galvez and clarkdo July 29, 2019 14:56
@@ -20,10 +20,13 @@ export default class WebpackServerConfig extends WebpackBaseConfig {
const whitelist = [
/\.(?!js(x|on)?$)/i
]
for (const pattern of this.buildContext.buildOptions.transpile) {
for (let pattern of this.buildContext.buildOptions.transpile) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we have a duplicate code block here (as in src/config/base.js, L61-70). Could we extract it somehow?

Copy link
Member

Choose a reason for hiding this comment

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

I will do it in another pr since this has been a long time

@clarkdo clarkdo merged commit e8f1532 into dev Aug 3, 2019
@clarkdo clarkdo deleted the fix/transpile-if branch August 3, 2019 20:09
@pi0 pi0 mentioned this pull request Aug 5, 2019
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.

7 participants