Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Before #2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib. We can duplicate all existing .js
files and replace their extension with .es6.js
. This is a setup phase to enable development with ES6 build tooling.
Comment | File | Size | Author |
---|---|---|---|
#41 | 2818825-41.patch | 1.07 MB | GrandmaGlassesRopeMan |
#40 | 2818825-40.patch | 1.98 MB | GrandmaGlassesRopeMan |
#38 | 2818825-38.patch | 1.07 MB | GrandmaGlassesRopeMan |
#31 | 2818825-31.patch | 1.05 MB | GrandmaGlassesRopeMan |
Comments
Comment #2
nod_big rename & compile. Should probably wait to have a good change record before moving on this.
Comment #3
nod_Comment #5
GrandmaGlassesRopeManThis patch duplicates all JavaScript files in
{misc,modules,themes}
, replacing their extension with,.es6.js
.This should be committed before #2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib, which will add ES6 build tooling enabling us to effectively ignore
*.js
files.Comment #6
GrandmaGlassesRopeManComment #8
GrandmaGlassesRopeManComment #9
GrandmaGlassesRopeManThis is exactly currently as of
6154128112
.Comment #10
GrandmaGlassesRopeManAfter talking with @alexpott I think it makes sense to include a 'built' copy of each of these files. Additionally a comment is added to the top of the built copy indicating to not edit the built copy.
Comment #11
nod_If we're not minifing, we need to get rid of comments in the transpiled files at least. Adding
@preserve
to the comment header will tell babel to keep the comment around.Side note, the lint:core-js shouldn't auto fix issues. testbot won't complain but it won't be right either way.
The rest looks fine I guess. as long as the copy script was used we shouldn't have problems.
Comment #12
GrandmaGlassesRopeManAlright, comment removal is back! Additionally the header note is being kept, thanks @nod_. Lets create a followup to remove the
--fix
part of linting?Comment #13
nod_Everything looks good to me.
We'll get another huge round of code standard fix :p
Not sure who can RTBC this though :p
Comment #14
GrandmaGlassesRopeMan@droplet
Can you have a look at this? Thanks.
Comment #15
droplet CreditAttribution: droplet commentedMy Review Process:
==========
Should we add
cross-env NODE_ENV=production
to build:js ?Comment #16
GrandmaGlassesRopeMan@droplet
The changes to
content_types.js
is expected since this patch contains the original JavaScript passed through the transpiler, which at this point only removes the comments, leaving a note to edit only the.es6.js
file.We don't have any specific build steps that would require a
production
flag. Perhaps if we decide to minify the output later we can add that?Comment #17
droplet CreditAttribution: droplet commented@drpal
this is a recent change:
https://www.drupal.org/node/2849100#comment-12084807
Cool. No more default to development:Misread. It still default to `development` and deprecating env in 7.0: https://github.com/babel/babel/issues/5276https://github.com/babel/babel/pull/4924/files
Any other better way to educate our developers to use this command:
Comment #18
droplet CreditAttribution: droplet commentedAlso,
please create Git diff with -C flag.
git diff -C
Thanks
Comment #19
GrandmaGlassesRopeMan@droplet
We still want to specify the environment since that dictates if we are going to generate sourcemaps or not. I'll update the change record to include the specifics around using the
--files
flag for building. And a new patch with-C
Comment #20
GrandmaGlassesRopeManChange record updated with some documentation about building only a single file, [#2873849].
Comment #21
droplet CreditAttribution: droplet commented#19 still using old version of
modules/node/content_types.js
Comment #22
droplet CreditAttribution: droplet commentedComment #23
GrandmaGlassesRopeMan@droplet
What do you mean?
Comment #24
droplet CreditAttribution: droplet commentedPatch contains the outdated version
Comment #25
GrandmaGlassesRopeMan@droplet
Thanks, I'll re-roll against the newest head.
Comment #26
droplet CreditAttribution: droplet commentedgit diff --cached -C --stat | grep es6.js
this command should output ZERO changes.
Comment #27
GrandmaGlassesRopeManAlright, this was created as of
5184e99d3f
.Comment #28
droplet CreditAttribution: droplet commentedMissing the banner. otherwise, it looks good. Thanks!
Comment #29
GrandmaGlassesRopeMan@droplet
My fault, I forgot that the header changes were not included till now. This is still current with
5184e99d3f
.Comment #30
droplet CreditAttribution: droplet commentedOne more @drpal. It should not add package.json to root
new file: ../package.json
Comment #31
GrandmaGlassesRopeMan@droplet
First try.
Comment #32
droplet CreditAttribution: droplet commentedGo Go GO!
Comment #33
GrandmaGlassesRopeManU+1F64F
Comment #34
nod_RTBC +1
Comment #35
nod_For information a side-effect of removing comments removes about 150KB–200KB of JS on all pages. I checked with minification, it helps a little but not as much.
Comment #36
cilefen CreditAttribution: cilefen commentedCould the “DO NOT EDIT THIS FILE.” comment link to the CR?
nit: there is an annoying space after "DO NOT EDIT THIS FILE"
I am basically a +1 for this and I like the enthusiasm. It's difficult to make sure code reflows didn't break something but I guess that is far-fetched. We'll have to open a PR on https://github.com/alexpott/d8githooks to eslint the es6 files going forward.
Comment #37
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedworks as expected.
Comment #38
GrandmaGlassesRopeManA minor update to the comment at the top of the built JavaScript files to include a link to the change record.
Comment #39
cilefen CreditAttribution: cilefen commented#2831274: Bring Media entity module to core as Media module added two new JS files.
Comment #40
GrandmaGlassesRopeManRerolled as of
01621e5880
.Comment #41
GrandmaGlassesRopeManMy fault, missed the
-C
flag when creating the patch. Exactly the same as the patch in #40.Comment #42
droplet CreditAttribution: droplet commentedHmmm.. #2831274: Bring Media entity module to core as Media module re-introduced a common JS bug (and bad code style) into CORE I think..
RTBC again. Other wording, banner things we able to fix in a follow-up. Let's get rid of a huge patch first :)
Comment #43
alexpottCommitted 8287017 and pushed to 8.4.x. Thanks!
Comment #46
Gábor HojtsyOpened #2888877: [PP-2] Update documentation following airbnb javascript style guide v13 adoption as a drupal.org docs followup.