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

examples(coffeescript): update module #6248

Merged
merged 3 commits into from
Aug 20, 2019
Merged

examples(coffeescript): update module #6248

merged 3 commits into from
Aug 20, 2019

Conversation

devartyom
Copy link

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

Resolves #5423

Checklist:

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

@@ -44,7 +44,7 @@ export default class Builder {
restart: null
}

this.supportedExtensions = ['vue', 'js', ...(this.options.build.additionalExtensions || [])]
this.supportedExtensions = ['vue', 'js', ...(this.options.extensions || []), ...(this.options.build.additionalExtensions || [])]
Copy link
Member

Choose a reason for hiding this comment

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

Are these options identical now? /cc @kevinmarrec

Copy link
Contributor

Choose a reason for hiding this comment

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

@pi0 No
https://github.com/nuxt/typescript/blob/master/packages/typescript-build/lib/module.js#L18
We need to be aware that the 2 options aren't used at the same places in core

Copy link
Contributor

Choose a reason for hiding this comment

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

options.extensions is more or less related to runtime, meanwhile the others only buildtime

@codecov-io
Copy link

codecov-io commented Aug 19, 2019

Codecov Report

Merging #6248 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #6248   +/-   ##
=======================================
  Coverage   95.72%   95.72%           
=======================================
  Files          80       80           
  Lines        2666     2666           
  Branches      687      687           
=======================================
  Hits         2552     2552           
  Misses         98       98           
  Partials       16       16
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.63% <ø> (ø) ⬆️
#unit 92.42% <ø> (ø) ⬆️

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 b22e054...c1e8c0f. Read the comment docs.

@pi0 pi0 requested a review from kevinmarrec August 19, 2019 07:58
@clarkdo
Copy link
Member

clarkdo commented Aug 19, 2019

Any reason for not using additionalExtensions? If extensions cover additionalExtensions, probably we should remove the duplicate one.

@kevinmarrec
Copy link
Contributor

kevinmarrec commented Aug 19, 2019

@pi0 @clarkdo If I created a new option it's cause there was a particular need.

All i know is that options.extensions is used to resolves files :
https://github.com/nuxt/nuxt.js/blob/dev/packages/config/src/options.js#L164
https://github.com/nuxt/nuxt.js/blob/dev/packages/core/src/resolver.js#L95

And options.build.additionalExtensions is used to build files that are not .vue or .js around Builder, or mostly create the router from files having these extensions.

This requested change https://github.com/nuxt/nuxt.js/pull/6248/files#diff-4f235fc89b94513b32c2cc8b5896cc5aR47 , would make js duplicated + make Builder build .mjs which Idk if it's relevant.

Having only one extensions would mean that the TypeScript module would make Nuxt also trying to resolve .tsx which shouldn't be needed ?

Idk

EDIT :
Current extensions when using TypeScript module :
options.extensions (for resolvePath core function) : ['js', 'mjs'] + ['ts'] = ['js', 'mjs', 'ts']
builder/router supported extensions : ['vue', 'js'] + additionalExtensions (['ts', 'tsx']) = ['vue', 'js', 'ts', 'tsx']

@pi0
Copy link
Member

pi0 commented Aug 19, 2019

@kevinmarrec Seems reasonable to have separated ones because of tsx :)

pi0
pi0 previously approved these changes Aug 19, 2019
@pi0 pi0 requested a review from clarkdo August 19, 2019 17:50
@clarkdo
Copy link
Member

clarkdo commented Aug 19, 2019

Sorry, I didn’t mean that we need to keep only one config, I mean that why we need to merge extensions into supportedExtensions instead of configuring additionalExtensions. For the coffeescript case, only adding this won’t work, user still needs to setup loader. And current change also changed the resolving order and files of routes, it’s kind of breaking change for users having files inside pages dir but not meant to be a route.

@kevinmarrec
Copy link
Contributor

kevinmarrec commented Aug 19, 2019

Exactly @clarkdo, cofeescript support for routes should leverage build.additionalExtensions within the cofeescript module.

If somehow cofeescript files should be resolved for other files like serverMiddlewares or local modules, then it should be done like TypeScript module by using both of config extensions options :

https://github.com/nuxt/typescript/blob/master/packages/typescript-build/lib/module.js#L18

@pi0
Copy link
Member

pi0 commented Aug 19, 2019

The breaking change seems introduced by 2.4.5...2.5.1 -- we are reverting :)

@kevinmarrec
Copy link
Contributor

kevinmarrec commented Aug 19, 2019

@pi0 AFAIK routes supported extensions never used options before. It was hardcoded, what breaking change was introduced ?

@pi0
Copy link
Member

pi0 commented Aug 19, 2019

According to #5423 CoffeeScript support was working and stopped working. But yeah you are right it was hardcoded. src so strange...

@kevinmarrec
Copy link
Contributor

According the issue, a partial fix was found making Vuex World but not local modules, which kind of relevant as to have coffee work everywhere it nows need to be added in both extensions options

I think the fix it just about adding .coffee extension to build.additionalExtensions and nothing more.

There was anyway Indeed something changed in internals. Once upon a time there was a resolveFiles that seemed merging options.extensions inside supportedExtensions of the class that was storint this resolveFiles function

@pi0 pi0 self-requested a review August 19, 2019 18:36
@kevinmarrec
Copy link
Contributor

@pi0 LGTM now, juste need title update

@devartyom devartyom changed the title fix: add options.extensions to define which extensions are used fix: change examples of adding support for .coffee extension Aug 19, 2019
@pi0 pi0 changed the title fix: change examples of adding support for .coffee extension examples(coffeescript): update module Aug 20, 2019
@pi0 pi0 merged commit a6d60d1 into nuxt:dev Aug 20, 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.

Coffeescript Vuex silently stop working on 2.5.1
6 participants