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

refactor: add Vite compatibility when lazy loading #1084

Merged
merged 14 commits into from
Mar 4, 2021

Conversation

farnabaz
Copy link

@farnabaz farnabaz commented Mar 3, 2021

Using the module with Nuxt Vite does not lazy load languages on client site because of limitations of dynamic import.

This PR will expand dynamic import into a switch case to remove variables inside import name and automatically detects build target and change the srcDir alias

@pi0 What do you think about this?

@farnabaz farnabaz marked this pull request as draft March 3, 2021 10:29
@rchl
Copy link
Collaborator

rchl commented Mar 3, 2021

I'm fine with it although I would love if there wouldn't have to be a special case checking for the presence of nuxt-vite. Do you have some links that explain that @fs{dir} syntax?

@farnabaz
Copy link
Author

farnabaz commented Mar 3, 2021

I would love if there wouldn't have to be a special case checking for the presence of nuxt-vite

I asked pooya to check it, I wanted to make sure about checking vite environment here (He may have better idea):

/* <% const _srcDir = (isDev && (nuxtOptions.buildModules || []).includes('nuxt-vite')) ? '/@fs' + srcDir : '~' %> */

Do you have some links that explain that @fs{dir} syntax?

It is the second reason why I keep this drafted and asked Pooya. @fs is not documented and I found it in Vite internals.

@farnabaz
Copy link
Author

farnabaz commented Mar 3, 2021

My bad, your refactor is great :) I was addressing isDev && (nuxtOptions.buildModules || []).includes('nuxt-vite'))

@rchl
Copy link
Collaborator

rchl commented Mar 3, 2021

Ideally, Vite would also support the ~ alias since it's used in many places. I'm surprised if it doesn't.

(sorry, I've removed my earlier comment as I've realized that you meant something else)

@pi0
Copy link

pi0 commented Mar 3, 2021

@farnabaz @rchl At least at this stage, we should not depend on any vite-specific feature like checking it's existence from buildModules or @fs/ requests.

Ideally, Vite would also support the ~ alias since it's used in many places

Actually it should do since we are providing resolve.alias option for ~ (otherwise it is a bug in either vite or nuxt-vite)

Let me check PR locally :)

src/templates/utils.js Outdated Show resolved Hide resolved
src/templates/options.js Outdated Show resolved Hide resolved
src/templates/options.js Outdated Show resolved Hide resolved
src/templates/options.js Outdated Show resolved Hide resolved
src/templates/options.js Outdated Show resolved Hide resolved
src/templates/options.js Outdated Show resolved Hide resolved
@rchl rchl marked this pull request as ready for review March 3, 2021 13:20
@rchl
Copy link
Collaborator

rchl commented Mar 3, 2021

Thanks to both of you for your work on that.

src/templates/options.js Outdated Show resolved Hide resolved
<%= options.locales.map(l => `'${l.file}': () => import('../${relativeToBuild(options.langDir, l.file)}' /* webpackChunkName: "lang-${l.file}" */)`).join(',\n ') %>
<%= options.locales
.map(l => `'${l.file}': () => import('../${relativeToBuild(options.langDir, l.file)}' /* webpackChunkName: "lang-${l.file}" */)`)
.filter((l, index, self) => self.indexOf(l) === index)
Copy link

Choose a reason for hiding this comment

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

This could be done in a simpler approach by mapping to file and use a Set 😃

Copy link

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Needs to tested but looks good to me now

Copy link
Collaborator

@rchl rchl left a comment

Choose a reason for hiding this comment

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

LGTM too. I've tested it in one of my projects that uses lazy loading and of course tests pass also.

@rchl
Copy link
Collaborator

rchl commented Mar 3, 2021

Haven't tested with Vite though so I can wait until someone does that.

@pi0
Copy link

pi0 commented Mar 3, 2021

Haven't tested with Vite though so I can wait until someone does that.

There is no pressure on vite. Since everything is super experimental know so i don't want to make chaos in community for vite support. These PRs generally improved runtime quality with esm support.

@rchl rchl changed the title fix: refactor lazy imports to make and support Vite refactor: add Vite compatibility when lazy loading Mar 4, 2021
@rchl rchl merged commit 9b357da into nuxt-modules:master Mar 4, 2021
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.

3 participants