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: 🎸 ESM Loading. Remove System JS. IE Support. CSS Hydration Fixes. Naming Conflict fixes. Robust support for plugins. #42

Merged
merged 55 commits into from
Oct 29, 2020

Conversation

nickreese
Copy link
Contributor

@nickreese nickreese commented Sep 24, 2020

This add the ability to split components to dev which will speed up local dev but breaks stores.

Usage:

module.exports = [...getRollupConfig({ svelteConfig, 
rollupConfig: { 
dev: { splitComponents: true } 
} 
})];

Also got a working module/nomodule setup built using iifes that doesn't break the Elder.js apis.

Testing

To test this:

  1. clone this branch and type npm install && npm link
  2. degit the template with npx degit Elderjs/template elderjs-app
  3. cd elderjs-app
  4. Update the @elderjs/elderjs entry in the package.json to be file:../relative/path/here/ and then type npm install.
  5. npm run build

Notes:

@nickreese nickreese mentioned this pull request Sep 26, 2020
@nickreese nickreese marked this pull request as draft September 28, 2020 18:33
@jbmoelker
Copy link

Very curious about this feature. Just putting some findings here first, without having looked at the code:

  • Test step 4, you can use a linked package using npm link @elderjs/elderjs inside elderjs-app/.
  • npm run rollup:dev no longer works without rollupConfig: { dev: { splitComponents: true } }. Can that become optional?
  • Maybe it's odd to have a rollupConfig property in getRollupConfig? Could this be getRollupConfig({ dev: { ... }, svelteConfig })?
  • When I run dev:rollup and dev:server and I open http://localhost:3000/ I get a 404 on http://localhost:3000/svelte/undefined.mjs and the clock doesn't hydrate. Is it supposed to do that? If I set { splitComponents: false } it loads and hydrates again. Without SystemJS, so 🥳.

When I run a build and serve it locally using npx serve public/, the page works both in modern browsers and IE11. 💯

So quick first verdict would be that this should make it in :) Maybe without the dev: { splitComponents: true } to start with?

Copy link

@jbmoelker jbmoelker left a comment

Choose a reason for hiding this comment

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

Complex stuff. Great that you're digging into this. I can't think of a simpler solution. So for now, just left some small comments.

I do really enjoy seeing the module/nomodule script mechanism at work. As I think it's a really lean solution for wide browser support.

src/utils/Page.ts Outdated Show resolved Hide resolved
src/utils/__tests__/svelteComponent.spec.ts Outdated Show resolved Hide resolved
src/utils/getHashedSvelteComponents.ts Outdated Show resolved Hide resolved
src/utils/getHashedSvelteComponents.ts Outdated Show resolved Hide resolved
src/utils/getHashedSvelteComponents.ts Show resolved Hide resolved
src/utils/svelteComponent.ts Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 14, 2020

Codecov Report

Merging #42 into master will increase coverage by 0.74%.
The diff coverage is 92.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   92.65%   93.39%   +0.74%     
==========================================
  Files          32       38       +6     
  Lines        1198     1333     +135     
  Branches      253      278      +25     
==========================================
+ Hits         1110     1245     +135     
+ Misses         88       86       -2     
- Partials        0        2       +2     
Impacted Files Coverage Δ
src/hookInterface/hookInterface.ts 100.00% <ø> (ø)
src/hooks.ts 92.77% <ø> (-0.18%) ⬇️
src/utils/validations.ts 79.03% <66.66%> (-2.33%) ⬇️
src/utils/svelteComponent.ts 81.42% <78.26%> (-5.02%) ⬇️
src/Elder.ts 87.71% <84.88%> (-2.61%) ⬇️
src/utils/getRollupConfig.ts 95.12% <93.47%> (+8.66%) ⬆️
src/utils/rollupPluginHandleCss.ts 95.55% <95.55%> (ø)
src/plugins/index.ts 95.91% <95.91%> (ø)
src/routes/routes.ts 96.15% <100.00%> (+0.10%) ⬆️
src/utils/Page.ts 91.01% <100.00%> (+0.42%) ⬆️
... and 13 more

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 b185635...89e7a06. Read the comment docs.

@nickreese
Copy link
Contributor Author

@halafi @jbmoelker @decrek I think this is ready to merge in and release. 👍

Filip, we could use some tests for some of the new features/improvements when you have downtime.

@nickreese nickreese linked an issue Oct 14, 2020 that may be closed by this pull request
@nickreese nickreese changed the title feat: 🎸 conditional esm/systemjs loading feat: 🎸 ESM Loading. Remove System JS. IE Support. CSS Hydration Fixes. Naming Conflict fixes Oct 28, 2020
@nickreese nickreese changed the title feat: 🎸 ESM Loading. Remove System JS. IE Support. CSS Hydration Fixes. Naming Conflict fixes feat: 🎸 ESM Loading. Remove System JS. IE Support. CSS Hydration Fixes. Naming Conflict fixes. Robust support for plugins. Oct 28, 2020
@thisislawatts
Copy link
Contributor

thisislawatts commented Oct 29, 2020

@nickreese this PR is a little hard for me to review given it's size. Have you considered rebasing against master branch, instead of merging master into esm to reduce some of noise here.
Are there any specific changes you would like extra eyes on?

@nickreese
Copy link
Contributor Author

nickreese commented Oct 29, 2020

@thisislawatts I’ll need to google rebase. 👀

This started as a refactor for ESM/ie11 bundles which deeply changed our rollup configs. Then I wanted to fix the CSS issues as it was closely ties with the rollup configs. The problem was that the string kept going as the only way to pull off the CSS implementation was to make sure there weren’t component name shadowing and even a deeper refactor of the rollup configs.

I’ll look at rebase. This is tested against the template and 2 production projects so I’m confident in the fact the code works. Just mainly realize that more eyes are better than fewer but overall I think it is ready to merge.

@nickreese nickreese merged commit f8e8f18 into master Oct 29, 2020
@nickreese nickreese deleted the esm branch June 6, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants