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

Remove CSS Option #9350

Merged
merged 9 commits into from Mar 8, 2019

Conversation

@wmarques
Copy link
Contributor

commented Feb 28, 2019

Fixes #8150

  • Please make sure the below checklist is followed for Pull Requests.

  • Travis tests are green

  • Tests are added where necessary

  • Documentation is added/updated where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@fleboulch

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

@wmarques what are the motivations to remove this option?

@wmarques

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@fleboulch This was discussed in #8150 .
The motivations are:

  • Reduce complexity by generating only one type of stylesheets
  • SCSS is far more used, the issues we had with node-sass are now resolved by the sass implementation in Dart

EDIT: Just saw i wrongly named my PR, we intend to remove CSS and not SCSS 😄

@wmarques wmarques changed the title Remove SCSS Option Remove CSS Option Mar 1, 2019
@deepu105

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@wmarques

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@deepu105 Yep I edited it 😄
I will probably need some help on the sub-gen part... What do we intend to do? An additional question if the user was using CSS ?

@fleboulch

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

After updating the title of your PR, I better understand what you want to do (I didn't look at the code but I was just wondering why SCSS would have been removed)!
👍

@wmarques wmarques force-pushed the wmarques:remove-scss branch from 39b6dbe to d843fce Mar 1, 2019
@wmarques

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Hope this will make the tests pass...
I'm going on little vacations so I won't be able to finish it until Tuesday included 😞

The things that has to be done IMO are:

  • Make CI green ofc
  • Add upgrade sub-gen support to handle version upgrade (don't know if this is needed as it's a major version so a breaking change isn't surprising for me)

If you want to help on this don't hesitate to push on my branch !

@deepu105

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

@DanielFran DanielFran referenced this pull request Mar 3, 2019
4 of 4 tasks complete
@jdubois

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

I restarted the build but it is still failing, it's weird there's an option with VueJS...

@wmarques

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

i'm back, let me check this :)

@wmarques wmarques force-pushed the wmarques:remove-scss branch from 60568e6 to 6904186 Mar 5, 2019
@wmarques wmarques referenced this pull request Mar 5, 2019
0 of 4 tasks complete
@wmarques

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

I don't know why the build is failing on a strange timeout error... @jhipster/developers does anyone have a clue on this?

@DanielFran

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Restarted the travis job failing...

@pascalgrimaud

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

The error is real: you could easily reproduce it locally with npm run test:unit test/upgrade.spec.js
Looks like it is stuck with these logs:

➜ npm run test:unit test/upgrade.spec.js

> generator-jhipster@5.8.2 test:unit /home/pgrimaud/projects/jhipster/generator-jhipster
> mocha --timeout 20000 --slow 0 --reporter spec "test/upgrade.spec.js"



  JHipster upgrade generator
    default application
Generating JHipster application in directory: /tmp/024198a6d2943f8ff14a1b7d4b35461b15dc8169
Upgrading the JHipster application
5.8.2
true
master
Basculement sur la nouvelle branche 'jhipster_upgrade'
npm WARN saveError ENOENT: no such file or directory, open '/tmp/024198a6d2943f8ff14a1b7d4b35461b15dc8169/package.json'
npm WARN enoent ENOENT: no such file or directory, open '/tmp/024198a6d2943f8ff14a1b7d4b35461b15dc8169/package.json'
npm WARN 024198a6d2943f8ff14a1b7d4b35461b15dc8169 No description
npm WARN 024198a6d2943f8ff14a1b7d4b35461b15dc8169 No repository field.
npm WARN 024198a6d2943f8ff14a1b7d4b35461b15dc8169 No README data
npm WARN 024198a6d2943f8ff14a1b7d4b35461b15dc8169 No license field.

+ generator-jhipster@5.8.2
added 513 packages from 330 contributors and audited 3743 packages in 40.8s
found 10 moderate severity vulnerabilities
  run `npm audit fix` to fix them, or `npm audit` for details
/tmp/024198a6d2943f8ff14a1b7d4b35461b15dc8169/node_modules/.bin
INFO! Using JHipster version installed globally
INFO! Running default command
INFO! Executing jhipster:app
INFO! Options: withEntities: true, force: true, skipInstall: true, skipGit: true, insight: false, from-cli: true, with-entities: true, skip-install: true, skip-git: true


        ██╗ ██╗   ██╗ ████████╗ ███████╗   ██████╗ ████████╗ ████████╗ ███████╗
        ██║ ██║   ██║ ╚══██╔══╝ ██╔═══██╗ ██╔════╝ ╚══██╔══╝ ██╔═════╝ ██╔═══██╗
        ██║ ████████║    ██║    ███████╔╝ ╚█████╗     ██║    ██████╗   ███████╔╝
  ██╗   ██║ ██╔═══██║    ██║    ██╔════╝   ╚═══██╗    ██║    ██╔═══╝   ██╔══██║
  ╚██████╔╝ ██║   ██║ ████████╗ ██║       ██████╔╝    ██║    ████████╗ ██║  ╚██╗
   ╚═════╝  ╚═╝   ╚═╝ ╚═══════╝ ╚═╝       ╚═════╝     ╚═╝    ╚═══════╝ ╚═╝   ╚═╝

                            https://www.jhipster.tech

Welcome to JHipster v5.8.2
Application files will be generated in folder: /tmp/024198a6d2943f8ff14a1b7d4b35461b15dc8169
 _______________________________________________________________________________________________________________

  Documentation for creating an application is at https://www.jhipster.tech/creating-an-app/
  If you find JHipster useful, consider sponsoring the project at https://opencollective.com/generator-jhipster
 _______________________________________________________________________________________________________________

This is an existing project, using the configuration from your .yo-rc.json file 
to re-generate the project...

? Which *Framework* would you like to use for the client? (Use arrow keys)
❯ Angular 
  React
@pascalgrimaud

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

I fixed it with: cb626ce
We need to keep useSass: true for now, otherwise, the user won't be able to upgrade their application.

To reproduce the original issue:

  • use v5.8.2 branch
  • try to generate the application with this config - useSass: true/false is missing:
{
  "generator-jhipster": {
    "jhipsterVersion": "5.8.2",
    "applicationType": "monolith",
    "baseName": "jhipster",
    "packageName": "com.mycompany.myapp",
    "packageFolder": "com/mycompany/myapp",
    "serverPort": "8080",
    "authenticationType": "jwt",
    "cacheProvider": "ehcache",
    "enableHibernateCache": true,
    "websocket": false,
    "databaseType": "sql",
    "devDatabaseType": "h2Memory",
    "prodDatabaseType": "mysql",
    "searchEngine": false,
    "messageBroker": false,
    "serviceDiscoveryType": false,
    "buildTool": "maven",
    "enableSwaggerCodegen": false,
    "jwtSecretKey": "ODE5MmQ0ZjQ1NjhkZjY1Nzc1YTMyNWJjNWZkZWZiZDdmZGMwZDZjN2YxYTlhOTRkMDQ0ZDQyYjFlMTJmNDJjMmQxMDQ2NjdiODkzNjJjOWE5ZmM0MzM5ZTljMjhkMzM1OWI5Y2ExYWMzMTUyNzk3NmMxZmNiOGZiNWJhY2YzZWU=",
    "clientFramework": "angularX",
    "clientPackageManager": "npm",
    "testFrameworks": [],
    "jhiPrefix": "jhi",
    "entitySuffix": "",
    "dtoSuffix": "DTO",
    "otherModules": [],
    "enableTranslation": true,
    "nativeLanguage": "en",
    "languages": [
      "en",
      "fr"
    ]
  }
}
  • you will be stucked at the question:
This is an existing project, using the configuration from your .yo-rc.json file 
to re-generate the project...

? Which *Framework* would you like to use for the client? (Use arrow keys)
❯ Angular 
  React 

That's the reason why useSass: true should still be saved in .yo-rc.json.
It can be removed later, once v6 is released :-)

@wmarques

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Wow thanks alot @pascalgrimaud !

@pascalgrimaud pascalgrimaud merged commit 4fac300 into jhipster:master Mar 8, 2019
1 check passed
1 check passed
jhipster.generator-jhipster Build #20190307.3 succeeded
Details
@jdubois jdubois added this to the 6.0.0-beta.0 milestone Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.